Skip to content

Conversation

@stephentoub
Copy link
Member

Fixes #88478

@ghost ghost added needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners new-api-needs-documentation labels Jul 19, 2023
@ghost
Copy link

ghost commented Jul 19, 2023

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost ghost assigned stephentoub Jul 19, 2023
@vargaz
Copy link
Contributor

vargaz commented Jul 19, 2023

The mono changes look ok.

@EgorBo
Copy link
Member

EgorBo commented Jul 19, 2023

JIT looks ok too, but VM seems to be missing changes in jitinterface.cpp e.g.

else if (tk == CoreLibBinder::GetMethod(METHOD__UNSAFE__BYREF_IS_ADDRESS_LESS_THAN)->GetMemberDef())
{
// Compare the two arguments
static const BYTE ilcode[] =
{
CEE_LDARG_0,
CEE_LDARG_1,
CEE_PREFIX1, (CEE_CLT_UN & 0xFF),
CEE_RET
};
setILIntrinsicMethodInfo(methInfo,const_cast<BYTE*>(ilcode),sizeof(ilcode), 2);
return true;
}
(LessThan) as an example.

Presumably, we don't have to implement them in R2R/CoreCLR with raw il code and leave only JIT version if we make them "must expand"/recursive? (but it's not related to your PR - we can do that sort of clean up separately if it's possible)

@stephentoub stephentoub added area-System.Runtime.CompilerServices and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 19, 2023
@ghost
Copy link

ghost commented Jul 19, 2023

Tagging subscribers to this area: @dotnet/area-system-runtime-compilerservices
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #88478

Author: stephentoub
Assignees: stephentoub
Labels:

area-System.Runtime.CompilerServices, new-api-needs-documentation, needs-area-label

Milestone: -

#define OP_PCLT_UN OP_LCLT_UN
#define OP_PCLE_UN OP_LCLE_UN
#define OP_PCGT_UN OP_LCGT_UN
#define OP_PCGE_UN OP_LCGE_UN
Copy link
Member Author

@stephentoub stephentoub Jul 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vargaz, it looks like while OP_ICLE_UN and OP_ICGE_UN exist, OP_LCLE_UN and OP_LCGE_UN don't. What would you recommend as the right way to implement this? Remove these defines and update intrinsics.c with an implementation that emits e.g. a cgt and ceq for less than or equal?

@stephentoub stephentoub added this to the 9.0.0 milestone Aug 9, 2023
@stephentoub stephentoub closed this Sep 6, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 6, 2023
@stephentoub stephentoub deleted the unsafelessgreaterequal branch December 12, 2025 23:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[API Proposal]: Unsafe.IsAddressLessThanOrEqualTo / IsAddressGreaterThanOrEqualTo

3 participants