[NativeAOT-LLVM]: Initial support for IsVirtualVtable calls #1768

yowl · 2021-12-05T19:13:38Z

This PR adds the start of support for CT_INDIRECT by supporting the most simple cases, when the target is known at compile time, and when the target is a simple indirect call from a variable.

To support this I've added a flag to the call node to indicate if there is a hidden parameter so that the signature can be generated correctly (similar to how this is handled)

Added a fix for a store op in between phi nodes which is illegal in LLVM. I thought I'd already done that but apparently not.

Included the delegates test as it passes

add flag to call to indicate if there is a hidden parameter include delegates test fix for store in between phi nodes

SingleAccretion · 2021-12-06T11:16:54Z

src/coreclr/jit/indirectcalltransformer.cpp

@@ -491,7 +494,14 @@ class IndirectCallTransformer
        }

    private:
+        // Wasm stores function pointers as indicies in a function table


Suggested change

// Wasm stores function pointers as indicies in a function table

// Wasm stores function pointers as indices in a function table.

SingleAccretion · 2021-12-06T11:41:24Z

src/coreclr/jit/llvm.cpp

+        // assume ExternalLinkage, if the function is defined in the clrjit module, then it is replaced and an
+        // extern added to the Ilc module
+        llvmFunc =
+            Function::Create(getFunctionTypeForCall(call), Function::ExternalLinkage, 0U, symbolName, _module);


It would be nice to rename getFunctionTypeForCall to createFunctionTypeForCall, for symmetry.

SingleAccretion · 2021-12-06T12:00:08Z

src/coreclr/jit/llvm.cpp

+                // For CT_INDIRECT calls, the callNode->callSig is present,
+                // other times it's not, so get the siginfo from the GenTree call node if possible, else from eeGetMethodSig
+                CORINFO_SIG_INFO* calleeSigInfo = callNode->callSig;
+                if (calleeSigInfo == nullptr)
+                {
+                    CORINFO_SIG_INFO eeCalleeSigInfo;
+                    _compiler->eeGetMethodSig(callNode->gtCallMethHnd, &eeCalleeSigInfo);
+                    calleeSigInfo = &eeCalleeSigInfo;


It is the case that for indirect calls the method handle is in a union with gtCallMethHnd:

union { CORINFO_METHOD_HANDLE gtCallMethHnd; // CT_USER_FUNC or CT_HELPER GenTree* gtCallAddr; // CT_INDIRECT };

So it is not clear how does this work to me.

In what cases is a nullptr signature seen? We should probably fix those. I note that the signature field is under #ifdef DEBUG, so it is not terribly surprising we can see it not preserved.

now we are not doing CT_INDIRECT but CALLV I'm just reverting this.

SingleAccretion · 2021-12-06T12:59:47Z

src/coreclr/jit/llvm.cpp

+    // calculate hiddenArg number if present
+    unsigned hiddenArgNum = 0;
+    if (callNode->gtHasHiddenArgument)
+    {
+        if (callThisArg != nullptr)
+        {
+            hiddenArgNum++;
+        }
+        if (callNode->HasRetBufArg())
+        {
+            hiddenArgNum++;
+        }
+    }


I actually wonder if we need gtHasHiddenArgument and this logic. It seems we will need to handle every arg-not-in-signature here, which seems unfortunate (we will essentially have to duplicate part of logic from fgMorphArgs).

We can exploit the fact that in the sorted args order, all non-user (and thus non-signature) args always come first. Subtracting the number of args in the signature from the number of args in the arg info should give us the threshold after which we can start using the signature for argument types.

You are right, I've removed it.

SingleAccretion · 2021-12-06T13:04:22Z

src/coreclr/jit/llvm.cpp

+        Function* llvmFunc = getOrCreateLlvmFunction(symbolName, call);
+        llvmFuncCallee = llvmFunc;
+    }
+    else if (call->gtCallType == CT_INDIRECT)


Suggested change

else if (call->gtCallType == CT_INDIRECT)

else if (call->gtCallType == CT_INDIRECT)

I would drop the else if, leaving only the else, and assert under it that the call is indirect.

This captures the idea that this method only builds 2 call types, and will only ever do that (well, it could eventually morph into just buildCall, but that'll be a different function).

SingleAccretion · 2021-12-06T13:09:27Z

src/coreclr/jit/llvm.cpp

+        if (call->gtEntryPoint.accessType == IAT_VALUE)
+        {
+            FunctionType* functionType = getFunctionTypeForCall(call);

-    for (GenTreeCall::Use& use : call->Args())
+            //TODO-LLVM: how best to detect there is no runtime lookup required when the target is a constant and not abstract or virtual
+            if (llvm::ConstantInt* llvmConstantInt = llvm::dyn_cast<llvm::ConstantInt>(funcValue)) {
+                void* methodHandle = (void*)(llvmConstantInt->getZExtValue());
+
+                const char* methodName = _getMangledSymbolNameFromHelperTarget(_thisPtr, methodHandle);
+                if (methodName == nullptr) // TODO-LLVM: abstract or virtual call
+                {
+                    failFunctionCompilation();
+                }
+
+                (*_addCodeReloc)(_thisPtr, methodHandle);
+
+                llvmFuncCallee = { functionType, getOrCreateLlvmFunction(methodName, call) };


We are effectively doing "devirtualization for indirect calls" here, manually.

Why?

Is the issue that we need to call (*_addCodeReloc)(_thisPtr, methodHandle);? It does not seem right to have it be "pulled" from under the call, it should instead be registered at the logical ldftn point, when building the the icon node.

Thanks, it's good that you've questioned this, as it gives something concrete to talk about. I didn't understand how to do the virtual calls so restricted myself to those calls that could be determined at runtime. The addCodeReloc is there to make sure we get the target method compiled. Happy to expand the PR to cover virtual calls but I didn't see how to make the slot lookup work, and I suspected I was missing something. How do the other Ilc backends do this I wonder?

@jkotas @MichalStrehovsky Maybe I can pull one of you in here please ?

@yowl let me test my understanding here. Is the following true?

addCodeReloc is only required for helpers. User methods are fine as they have been added as dependencies by the ILC's front-end.

Ummm.... If I delete the call to _addCodeReloc , then it works. Oh dear, sorry about that. I still could use some help on the slot lookup side though. Let me get some IR for that.

Yes, that looks right to me.

When we lower the call args, we remove the gtCallThisArg as its pushed to the shadowstack. I'm going to need that aren't I, to get to the method table?

I'm going to need that aren't I, to get to the method table?

Why? The idea here is that we don't need to do that, the control expression (== function address) has already been computed for us by morph.

Thanks, just me being slow.

Ok, cool, this is super simple now it seems, thanks.

if (call->IsVirtualVtable()) { FunctionType* functionType = createFunctionTypeForCall(call); Value* funcPtr = castIfNecessary(getGenTreeValue(call->gtControlExpr), functionType->getPointerTo()); llvmFuncCallee = {functionType, funcPtr}; }

yowl · 2021-12-08T15:18:06Z

@MichalStrehovsky Thanks, looking at

runtimelab/src/coreclr/tools/aot/ILCompiler.RyuJit/JitInterface/CorInfoImpl.RyuJit.cs

Lines 120 to 138 in dd8c43a

    
           lookup.runtimeLookup.indirections = (ushort)(genericLookup.NumberOfIndirections + (genericLookup.IndirectLastOffset ? 1 : 0)); 
        
           lookup.runtimeLookup.offset0 = (IntPtr)genericLookup[0]; 
        
           if (genericLookup.NumberOfIndirections > 1) 
        
           { 
        
               lookup.runtimeLookup.offset1 = (IntPtr)genericLookup[1]; 
        
               if (genericLookup.IndirectLastOffset) 
        
                   lookup.runtimeLookup.offset2 = IntPtr.Zero; 
        
           } 
        
           else if (genericLookup.IndirectLastOffset) 
        
           { 
        
               lookup.runtimeLookup.offset1 = IntPtr.Zero; 
        
           } 
        
           lookup.runtimeLookup.sizeOffset = CORINFO.CORINFO_NO_SIZE_CHECK; 
        
           lookup.runtimeLookup.testForFixup = false; // TODO: this will be needed in true multifile 
        
           lookup.runtimeLookup.testForNull = false; 
        
           lookup.runtimeLookup.indirectFirstOffset = false; 
        
           lookup.runtimeLookup.indirectSecondOffset = false; 
        
           lookup.lookupKind.runtimeLookupFlags = 0; 
        
           lookup.lookupKind.runtimeLookupArgs = null;

and

runtimelab/src/coreclr/jit/importer.cpp

Line 2230 in dd8c43a

    
           GenTree* Compiler::impRuntimeLookupToTree(CORINFO_RESOLVED_TOKEN* pResolvedToken,

the steps from the result of ComputeGenericLookup are:

For each indirection, indirect (starting from the context)
add the offset.
After repeating pRuntimeLookup->indirections number of times, indirect one more time

Does that sound right?

I'm going to #if it out so need to answer this really. I'll get back to it at some point.

MichalStrehovsky · 2021-12-09T02:21:53Z

After repeating pRuntimeLookup->indirections number of times, indirect one more time

Not sure that one is needed. It should match the indirections that are done for the ReadyToRunGenericLookupHelper (the LLVMSharp-based WASM backend already does all of this because it needs to generate the corresponding helper; it's just in a different location).

…ble slots (#1834) This PR enables the IL scanner for LLVM. The scanner is always on for LLVM to enable precomputed vtables slots. This will unblock the `CT_INDIRECT` work #1768 . At ComputeGenericLookup in `Compilation.cs` you can see that it never takes the fixed lookup path for generic calls, so not sure that if that is going to be a problem. There are some changes to the scanner to add dependencies required by the IL->LLVM compilation which should be removed when all code is generated by clrjit. Including - The scanner has the same stubbing out of methods not implemented for LLVM, `GetRandomBytes` and `EnumCalendarInfo` - For expanding intrinsics, the scanner has to add dependencies for both expanding and not expanding as it doesn't know which module will compile the method. - Adding dependencies for throwing exceptions and exception handling - Various other places where the IL->LLVM compilation differs from the scanner

…indirect

…table Remove gtHasHiddenArgument Addresses feedback

yowl · 2022-02-02T20:57:18Z

Latest commit addresses remaining feedback. Thanks for reviewing.

yowl · 2022-02-03T00:51:11Z

Disabling the Delegates test which I had enabled here. It fails in Release config.

jkotas · 2022-02-08T21:47:52Z

@SingleAccretion Could you please do final review of this change?

SingleAccretion · 2022-02-08T21:49:22Z

Sorry for the delay, will do so tomorrow.

SingleAccretion · 2022-02-09T09:52:00Z

src/coreclr/jit/llvm.cpp

+        if (use.GetNode() == call->gtControlExpr)
+            continue;


When is this case hit?

The control expression should never be in the args (in fact, it would violate the "single use" invariant of an SDSU).

Never apparently. Have deleted it.

SingleAccretion · 2022-02-09T09:55:17Z

src/coreclr/jit/llvm.cpp

@@ -1026,7 +1058,7 @@ void Llvm::buildCall(GenTree* node)
    {
        buildHelperFuncCall(call);
    }
-    else if (call->gtCallType == CT_USER_FUNC && !call->IsVirtualStub() /* TODO: Virtual stub not implemented */)
+    else if ((call->gtCallType == CT_USER_FUNC || call->gtCallType == CT_INDIRECT) && !call->IsVirtualStub() /* TODO: Virtual stub not implemented */)


call->gtCallType == CT_INDIRECT no longer needed (as it is no longer handled)?

Thanks, deleted.

src/coreclr/jit/llvm.cpp

SingleAccretion · 2022-02-09T10:14:03Z

src/coreclr/jit/llvm.cpp

@@ -1988,7 +2017,7 @@ void Llvm::lowerCallToShadowStack(GenTreeCall* callNode, CORINFO_SIG_INFO& calle
    GenTreeCall::Use* callThisArg = callNode->gtCallThisArg;

    callNode->ResetArgInfo();
-    callNode->gtCallThisArg = nullptr;
+    //callNode->gtCallThisArg = nullptr;


Why comment it out?

The way I read the code, it will cause a call node to reference the this SDSU twice (e. g. for VisitOperands).

(If some dumping code relies on it - we should adjust it)

mm, Have reinstated it, thanks

SingleAccretion · 2022-02-09T10:14:59Z

src/coreclr/jit/llvm.cpp

@@ -2014,26 +2043,39 @@ void Llvm::lowerCallToShadowStack(GenTreeCall* callNode, CORINFO_SIG_INFO& calle
    for (unsigned i = 0; i < argCount; i++)
    {
        fgArgTabEntry* curArgTabEntry = argTable[i];
+        if (curArgTabEntry->nonStandardArgKind == NonStandardArgKind::VirtualStubCell)


Cleaner to catch it in failUnsupportedCalls?

yes, that's better

SingleAccretion · 2022-02-09T10:19:00Z

src/coreclr/jit/llvm.cpp


    for (OperandArgNum opAndArg : sortedArgs)
    {
        CORINFO_CLASS_HANDLE clsHnd      = NO_CLASS_HANDLE;
        CorInfoType          corInfoType = CORINFO_TYPE_UNDEF;

        // "this" not in sigInfo arg list
-        bool isThis = callThisArg != nullptr && opAndArg.argNum == 0 && calleeSigInfo.hasThis();
-        if (!isThis)
+        bool isThis = callThisArg != nullptr && opAndArg.argNum == 0 && calleeSigInfo->hasThis();


I think it would be a little more robust to check against callThisArg:

isThis = callThisArg != nullptr && callThisArg->GetNode() == opAndArg.operand.

That is not quite the same. E.g. for the IR

------------ BB01 [???..???), preds={} succs={BB02} N001 ( 0, 0) [000000] ------------ NOP void ------------ BB02 [000..00B) -> BB04 (cond), preds={BB01} succs={BB03,BB04} [000021] ------------ IL_OFFSET void IL offset: 0x0 N001 ( 1, 1) [000001] ------------ NO_OP void [000022] ------------ IL_OFFSET void IL offset: 0x1 N002 ( 3, 2) [000002] ------------ t2 = LCL_VAR byref V00 this /--* t2 byref this in rcx N003 ( 17, 6) [000003] --CXG------- t3 = * CALL int System.Threading.CancellationToken.get_IsCancellationRequested /--* t3 int N004 ( 18, 8) [000004] ---XG------- t4 = * CAST int <- bool <- int /--* t4 int N006 ( 22, 11) [000006] DA-XG------- * STORE_LCL_VAR int V03 tmp1 [000023] ------------ IL_OFFSET void IL offset: 0x7 N001 ( 3, 2) [000007] ------------ t7 = LCL_VAR int V03 tmp1 /--* t7 int N002 ( 4, 4) [000019] ------------ t19 = * CAST int <- bool <- int /--* t19 int N004 ( 8, 7) [000009] DA---------- * STORE_LCL_VAR int V01 loc0 [000024] ------------ IL_OFFSET void IL offset: 0x8 N001 ( 3, 2) [000010] ------------ t10 = LCL_VAR int V01 loc0 N002 ( 1, 1) [000011] ------------ t11 = CNS_INT int 0 /--* t10 int +--* t11 int N003 ( 5, 4) [000012] J------N---- t12 = * EQ int /--* t12 int N004 ( 7, 6) [000013] ------------ * JTRUE void ------------ BB03 [00B..012), preds={BB02} succs={BB04} [000025] ------------ IL_OFFSET void IL offset: 0xb N002 ( 3, 2) [000015] ------------ t15 = LCL_VAR byref V00 this /--* t15 byref this in rcx N003 ( 17, 6) [000016] --CXG------- * CALL void System.Threading.CancellationToken.ThrowOperationCanceledException [000026] ------------ IL_OFFSET void IL offset: 0x11 N001 ( 1, 1) [000017] ------------ NO_OP void ------------ BB04 [012..013) (return), preds={BB02,BB03} succs={} [000027] ------------ IL_OFFSET void IL offset: 0x12 N001 ( 0, 0) [000014] ------------ RETURN void

For the call at tree id 16, callThisArg->GetNode() is a GT_ARGPLACE whereas opAndArg.operand has become GT_IND at treed id 15.

Ahh, the ever-present late-arg-not-late arg distinction. You could obtain the real node from the arg info but that point I agree it is not worth the trouble.

Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>

… ct-indirect

yowl · 2022-02-09T19:55:26Z

The OSX build failed

      nativeaot/SmokeTests/Threading/Threading/Threading.sh [FAIL]
        Unhandled Exception: AssertionFailureException: Exception of type 'AssertionFailureException' was thrown.
           at Assert.True(Boolean) + 0x59
           at WaitSubsystemTests.VerifyWaitDuration(WaitHandle, Int32, Int32) + 0xfb
           at WaitSubsystemTests.WaitDurationTest() + 0x51
           at Runner.Main() + 0x81
           at Threading!<BaseAddress>+0x487b9f
           at Threading!<BaseAddress>+0x487c17
        /Users/runner/work/1/s/artifacts/tests/coreclr/OSX.x64.Debug/nativeaot/SmokeTests/Threading/Threading/Threading.sh: line 408: 13719 Abort trap: 6           $LAUNCHER $ExePath "${CLRTestExecutionArguments[@]}"
        
        Return code:      1

And

ld : warning : linker symbol '$ld$hide$os10.4$__Unwind_Backtrace' hides a non-existent symbol '__Unwind_Backtrace' [/Users/runner/work/1/s/src/tests/run.proj]
ld : warning : linker symbol '$ld$hide$os10.5$__Unwind_Backtrace' hides a non-existent symbol '__Unwind_Backtrace' [/Users/runner/work/1/s/src/tests/run.proj]
ld : warning : linker symbol '$ld$hide$os10.4$__Unwind_DeleteException' hides a non-existent symbol '__Unwind_DeleteException' [/Users/runner/work/1/s/src/tests/run.proj]
ld : warning : linker symbol '$ld$hide$os10.5$__Unwind_DeleteException' hides a non-existent symbol '__Unwind_DeleteException' [/Users/runner/work/1/s/src/tests/run.proj]

Doesn't seem relevant to this PR. See what else fails...

yowl · 2022-02-09T20:16:28Z

Will retry.

SingleAccretion

Nice!

jkotas · 2022-02-10T13:14:35Z

@yowl @SingleAccretion Thank you!

start support for CT_INDIRECT

65838f5

add flag to call to indicate if there is a hidden parameter include delegates test fix for store in between phi nodes

SingleAccretion reviewed Dec 6, 2021

View reviewed changes

address easy feedback

14a90b3

yowl mentioned this pull request Jan 19, 2022

[NativeAOT-LLVM]: Outstanding tasks tracking issue #1828

Open

30 tasks

yowl mentioned this pull request Jan 29, 2022

[NativeAOT-LLVM] Enable IL scanner for LLVM to enable precomputed vtable slots #1834

Merged

yowl added 2 commits February 2, 2022 07:38

Merge branch 'origin_clrjit-args_2' into ct-indirect

04f6dd2

Merge remote-tracking branch 'origin/feature/NativeAOT-LLVM' into ct-…

066c1e9

…indirect

yowl changed the title ~~[NativeAOT-LLVM]: Initial support for CT_INDIRECT~~ [NativeAOT-LLVM]: Initial support for IsVirtualVtable calls Feb 2, 2022

ILScanner now enabled. Remove CT_INDIRECT and replace with IsVirtualV…

6e6ffb5

…table Remove gtHasHiddenArgument Addresses feedback

disable delegates test as fails in Release config

5b28fc1

yowl mentioned this pull request Feb 8, 2022

[NativeAOT-LLVM] Enable support for WASI SDK #1850

Closed

SingleAccretion reviewed Feb 9, 2022

View reviewed changes

yowl and others added 3 commits February 9, 2022 09:59

Update src/coreclr/jit/llvm.cpp

dc0ff2e

Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>

address feedback

5b3238d

Merge branch 'ct-indirect' of https://github.com/yowl/runtimelab into…

7e0178c

… ct-indirect

yowl closed this Feb 9, 2022

yowl reopened this Feb 9, 2022

SingleAccretion approved these changes Feb 10, 2022

View reviewed changes

jkotas merged commit ea61843 into dotnet:feature/NativeAOT-LLVM Feb 10, 2022

yowl deleted the ct-indirect branch February 10, 2022 13:43

	// Wasm stores function pointers as indicies in a function table
	// Wasm stores function pointers as indices in a function table.

	else if (call->gtCallType == CT_INDIRECT)
	else if (call->gtCallType == CT_INDIRECT)

[NativeAOT-LLVM]: Initial support for IsVirtualVtable calls #1768

[NativeAOT-LLVM]: Initial support for IsVirtualVtable calls #1768

Conversation

yowl commented Dec 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SingleAccretion Dec 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yowl commented Dec 8, 2021 • edited Loading

MichalStrehovsky commented Dec 9, 2021

yowl commented Feb 2, 2022

yowl commented Feb 3, 2022

jkotas commented Feb 8, 2022

SingleAccretion commented Feb 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SingleAccretion Feb 9, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yowl commented Feb 9, 2022

yowl commented Feb 9, 2022

SingleAccretion left a comment

Choose a reason for hiding this comment

jkotas commented Feb 10, 2022

SingleAccretion Dec 6, 2021 •

edited

Loading

yowl commented Dec 8, 2021 •

edited

Loading

SingleAccretion Feb 9, 2022 •

edited

Loading