[NativeAOT-LLVM] Merge may24

[yowl]

This PR merges to 301604e

• Some #ifdef BACKGROUND_GC in gc.cpp upstreamable.
• Adds a WasmClassifier
• Remove use of REG_R0/REG_F0 to REG_STK to avoid asserts arising from the classifier.
• #if out the assert(dscStackOffset == expected.GetStackOffset()); assert as "stack" offsets not aligned. If aligned we hit another assert as per the comment.
• Reorder REG_STK and REG_LLVM to avoid an assert
• A workaround for mono's use of emsdk_env.cmd where we have emsdk_env.bat , didn't look to see why mono has a different extension.
• comment out macos debug build: upstream issue.

[yowl]

cc @dotnet/nativeaot-llvm - should go green from here.

[SingleAccretion]

This should go upstream (probably the declarations should be ifdefed out too).

[SingleAccretion]

This one should go to the above group now with // Not used on WASM.. We could start using it too.

[SingleAccretion]

REG_STK should be the last register:

; target.h

// Each register list in register.h must declare REG_STK as the last value.

[SingleAccretion]

This will allocate two arrays, while only one is needed.

[SingleAccretion]

This will double the allocations on this path, which is not great. It would be better to re-expose UnderlyingArray instead.

[SingleAccretion]

Why does the second assert fail?

[yowl]

Good question, I can change the classifier to

    if (type == TYP_STRUCT)
    {
        structPassingKind wbPassStruct;
        type = comp->m_llvm->GetArgTypeForStructWasm(structLayout->GetClassHandle(), &wbPassStruct);
    }

    assert(type != TYP_STRUCT);

    unsigned typeSize = genTypeSize(type);

    ABIPassingSegment segment = ABIPassingSegment::OnStack(m_stackArgSize, 0, typeSize);
    m_stackArgSize += AlignUp(typeSize, TARGET_POINTER_SIZE);

    return ABIPassingInformation::FromSegment(comp, segment);

But that doesn't match the values passed to SetStackOffset
For example, when the first arg is not a "user arg", e.g. lvaInitThisPtr, then we dont set the stack offset, as its assumed to be allocRegArg(TYP_INT) I suppose. Maybe this is where I've gone wrong in attempting to remove the use of the registers and I should look for another solution?

[SingleAccretion]

There is an assumption in the compiler that this is always passed in a register. The new classifier can accommodate this assumption like so:

if (wellKnownParam == WellKnownArg::ThisPointer)
{
    // Produce a register segment (by bringing back R0).
}

We can delete this later - when the old classification is phased out.

[yowl]

Thanks, this is solved with that. For TYP_STRUCT and implicit by refs I dont see how these 2 uses of the size are working:

Here the size is used as is, ignoring byref structs, and the value used to set the stack offset.

https://github.com/dotnet/runtime/blob/3eb94d1a816f941dd66f01e62c1af25c108014bc/src/coreclr/jit/lclvars.cpp#L696

However here

https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/lclvars.cpp#L1904

the segment is checked against the size adusted for storing byrefs as pointers. How do we handle this to both pass this assert and the one that checks the classifer offsets with the stack offsets ?

[SingleAccretion]

We would need to modify eeGetArgSize to harmonize these. But at this point I agree that commenting out the assert is simpler.

[yowl]

Ok, I left the this code in but can take it out as you say.

[SingleAccretion]

I have done some time thinking about this, and looking at dotnet/runtime#102098, and I think the better fix for this issue is to augment IsTargetIntrinsic to return true for these intrinsics.

return nullptr from impEstimateIntrinsic requires a corresponding ifdef in CoreLib, see https://github.com/dotnet/runtime/pull/102098/files#r1597779222. We have the sqrt intrinsic, so ReciprocalSqrtEstimate is expandable inline for WASM, and others have trivial expansions.

The actual CQ item here is to enable the better CQ for these intrinsic by actually exploiting the non-determinism, but it's a bit hard because LLVM doesn't have the same notion of intrinsics deterministic on "process granularity".

[yowl]

Changed Math to

        public static double ReciprocalSqrtEstimate(double d)
        {
#if MONO || TARGET_RISCV64 || TARGET_LOONGARCH64 || TARGET_WASM
            return 1.0 / Sqrt(d);
#else
            return ReciprocalSqrtEstimate(d);
#endif
        }

[SingleAccretion]

Why was it needed? It doesn't look expected to me.

[yowl]

Its not needed, but I thought that' what you refered to in "requires a corresponding ...". I will revert it

[SingleAccretion]

If we are deleting fake registers, it would be better to replace them with REG_NA instead of REG_STK.

REG_STK is the half-way point between real and fake registers itself :).

[yowl]

Ok, reverted basically. I added

  #define RBM_R0                   REG_NA
  #define RBM_F0                   REG_NA
  #define REG_F0                   REG_NA

Maybe I should just collapse the uses of RBM/REG_F0 ?

[SingleAccretion]

Suggested change

	reg = REG_STK;
	regMask = RBM_STK;
	reg = REG_NA;
	regMask = REG_NA;

If that runs into asserts, R0 will need to be physically brought back.

[yowl]

Have brought it back, just didn't seem to be a great idea, at least not in this PR.

[SingleAccretion]

Suggested change

	#ifdef TARGET_WASM
	else if (i == info.compThisArg)
	{
	wellKnownArg = WellKnownArg::ThisPointer;
	}
	#endif //TARGET_WASM

No longer needed now that we've decided on silencing out the assert.

[yowl]

Right, wasn't sure if it was "correct". Now removed.

[SingleAccretion]

Suggested change

	const regNumber intArgRegs[] = { REG_STK };
	const regNumber fltArgRegs[] = { REG_STK };
	const regNumber intArgRegs[] = { REG_NA };
	const regNumber fltArgRegs[] = { REG_NA };

[SingleAccretion]

RBM_<...> is a register mask, i. e. 1 << REG_<...>. So the right "nothing" value for it is zero, i. e. RBM_NONE.

Suggested change

	#define RBM_R0 REG_NA
	#define RBM_F0 REG_NA
	#define REG_F0 REG_NA
	#define RBM_R0 RBM_NONE
	#define RBM_F0 RBM_NONE
	#define REG_F0 REG_NA

[SingleAccretion]

Suggested change

	#define RBM_ALLFLOAT REG_F0
	#define RBM_ALLFLOAT RBM_F0

[SingleAccretion]

There are a number of cases where REG_ values have been changed to be defined as RBM_R0, which are masks. They should be changed back to register values (REG_NA if that works, brought back R0/F0 otherwise).

[yowl]

Reinstated R0

[yowl]

I'm assuming the git hub failure is an infrastructure problem and this is actually green.

[SingleAccretion]

LGTM, thank you!

[jkotas]

I'm assuming the git hub failure is an infrastructure problem and this is actually green.

Looking into it

[dotnet-policy-service]

1 file(s) have code issues.

File	Issues
.github/policies/resourceManagement.yml	Exception during deserialization. Failed to create an instance of type 'GitOps.PullRequestIssueManagement.Core.Primitives.Data.Frequencies.SearchFrequency'.. Cannot dynamically create an instance of type 'GitOps.PullRequestIssueManagement.Core.Primitives.Data.Frequencies.SearchFrequency'. Reason: Cannot create an abstract class.

_{Total execution time: 0.01 seconds}

[jkotas]

Thank you!