[RISC-V][JIT] Disable promotion for splitted args by clamp03 · Pull Request #87855 · dotnet/runtime

clamp03 · 2023-06-21T00:48:25Z

Fix an assert in src/coreclr/jit/regalloc.cpp line 133

Assertion failed 'inArgMask & RBM_ARG_REGS' because inArgReg is 64 and inArgMask is 0
Fixed two tests on CHECKED build (neither DEBUG nor RELEASE)
./JIT/Performance/CodeQuality/BenchmarksGame/regex-redux/regex-redux-1/regex-redux-1.sh
./JIT/Performance/CodeQuality/BenchmarksGame/regex-redux/regex-redux-5/regex-redux-5.sh

In PromoteStructVar, it sets lvIsRegArg to 1 even if its register is REG_STK. It calls updateRegStateForArg for REG_STK like below. Then it crashes in raUpdateRegStateForArg

runtime/src/coreclr/jit/lsrabuild.cpp

Lines 2270 to 2273 in f3b599e

    
           if (argDsc->lvIsRegArg) 
        
           { 
        
               updateRegStateForArg(argDsc); 
        
           }

Part of #84834
cc @wscho77 @HJLeee @JongHeonChoi @t-mustafin @alpencolt @gbalykov

ghost · 2023-06-21T00:48:37Z

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Fix an assert in src/coreclr/jit/regalloc.cpp line 133 on CHECKED build. (Not on DEBUG build)

Assertion failed 'inArgMask & RBM_ARG_REGS' because inArgReg is 64 and inArgMask is 0
Fixed two tests
./JIT/Performance/CodeQuality/BenchmarksGame/regex-redux/regex-redux-1/regex-redux-1.sh
./JIT/Performance/CodeQuality/BenchmarksGame/regex-redux/regex-redux-5/regex-redux-5.sh

In PromoteStructVar, it set lvIsRegArg to 1 even if its register is REG_STK. It calls updateRegStateForArg for REG_STK like below. Then it crashes in raUpdateRegStateForArg

runtime/src/coreclr/jit/lsrabuild.cpp

Lines 2270 to 2273 in f3b599e

    
           if (argDsc->lvIsRegArg) 
        
           { 
        
               updateRegStateForArg(argDsc); 
        
           }

Part of #84834
cc @wscho77 @HJLeee @JongHeonChoi @t-mustafin @alpencolt @gbalykov

Author:	clamp03
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

clamp03 · 2023-06-21T00:50:55Z

@shushanhf I think this can fix in LA too. Could you review? If you have any better solution, please let me know. Thank you.

shushanhf · 2023-06-21T01:08:04Z

@shushanhf I think this can fix in LA too. Could you review? If you have any better solution, please let me know. Thank you.

Thanks very much.
But these two cases are OK for LA64 without this PR, they are passed within Release and Debug mode.

./JIT/Performance/CodeQuality/BenchmarksGame/regex-redux/regex-redux-1/regex-redux-1.sh
./JIT/Performance/CodeQuality/BenchmarksGame/regex-redux/regex-redux-5/regex-redux-5.sh

Maybe you just modify the RISCV-64.

clamp03 · 2023-06-21T01:11:30Z

@shushanhf I think this can fix in LA too. Could you review? If you have any better solution, please let me know. Thank you.

Thanks very much. But these two cases are OK for LA64 without this PR, they are passed within Release and Debug mode.

./JIT/Performance/CodeQuality/BenchmarksGame/regex-redux/regex-redux-1/regex-redux-1.sh
./JIT/Performance/CodeQuality/BenchmarksGame/regex-redux/regex-redux-5/regex-redux-5.sh

Maybe you just modify the RISCV-64.

Sure. Thank you.

shushanhf · 2023-06-21T01:26:14Z

src/coreclr/jit/lclvars.cpp

+#ifdef TARGET_RISCV64
+                        if (varDsc->lvIsSplit)
+                        {
+                            assert(fieldRegNum == REG_STK);
+                            fieldVarDsc->lvIsRegArg = 0;
+                        }
+#endif // TARGET_RISCV64


Maybe you don't need add these for RISCV-64.

As the fieldRegNum == REG_STK that is enough.
The second field is passed by STK when lvIsSplit is true, so you just modify RISCV-64's codegen is OK.

I am so sorry. I cannot understand exactly what you mean.
You mean RISCV64 doesn't need this PR at all? Or modify RISCV64's codegen instead of this PR? Or replace if (varDsc->lvIsSplit) to `if (fieldRegNum == REG_STK)?
Could you please explain again?
Thank you so much.

Or modify RISCV64's codegen instead of this PR?

This is ok.

Okay. Then you mean modify RISCV64's codegen not this PR.
However, I don't know well. The assertion in RISCV64 occurs in linearscan phase which is earlier than codegen phase as I know.
If you don't mind, could you please give code locations in LOONGARCH64 for me? I want to fix in RISCV64 referenced on LOONGARCH64.

+ And the test works well in DEBUG and RELEASE build on RISCV64 too. Because DEBUG build doesn't use PromoteStructVar ~~and RELEASE build doesn't compile a method which has the issue. (System.IO.Enumeration.FileSystemEntry:Initialize)~~ RELEASE build doesn't compile System.Text.RegularExpressions.RegexParser:.ctor with optimization.

Thank you.

The assertion in RISCV64 occurs in linearscan phase which is earlier than codegen phase as I know.

Can you add the errors' log that you met ?

Could you add the backtrace info for this assert error ?

#0 0x0000004002b4dc32 in ?? () from /usr/riscv64-linux-gnu/lib/libc.so.6 #1 0x0000004002b18b8a in raise () from /usr/riscv64-linux-gnu/lib/libc.so.6 #2 0x0000004002b09048 in abort () from /usr/riscv64-linux-gnu/lib/libc.so.6 #3 0x000000400324af22 in PROCAbort (signal=6, siginfo=0x0) at /home/clamp/Work/dotnet/riscv64/src/coreclr/pal/src/thread/process.cpp:2528 #4 0x000000400324adf6 in RaiseFailFastException (pExceptionRecord=<optimized out>, pContextRecord=<optimized out>, dwFlags=<optimized out>) at /home/clamp/Work/dotnet/riscv64/src/coreclr/pal/src/thread/process.cpp:1267 #5 0x0000004003087df4 in FailFastOnAssert () at /home/clamp/Work/dotnet/riscv64/src/coreclr/utilcode/debug.cpp:60 #6 0x0000004003087bd4 in _DbgBreakCheck (szFile=0x6078d355a0 "/home/clamp/Work/dotnet/riscv64/src/coreclr/jit/regalloc.cpp", iLine=133, szExpr=0x6079a4d550 "Assertion failed 'inArgMask & RBM_ARG_REGS' in 'System.Text.RegularExpressions.RegexParser:.ctor(System.String,int,System.Globalization.CultureInfo,System.Collections.Hashtable,int,System.Collections."..., fConstrained=<optimized out>) at /home/clamp/Work/dotnet/riscv64/src/coreclr/utilcode/debug.cpp:288 #7 0x0000004002d60852 in CEEInfo::doAssert (this=<optimized out>, szFile=0x6078d355a0 "/home/clamp/Work/dotnet/riscv64/src/coreclr/jit/regalloc.cpp", iLine=133, szExpr=0x6079a4d550 "Assertion failed 'inArgMask & RBM_ARG_REGS' in 'System.Text.RegularExpressions.RegexParser:.ctor(System.String,int,System.Globalization.CultureInfo,System.Collections.Hashtable,int,System.Collections."...) at /home/clamp/Work/dotnet/riscv64/src/coreclr/vm/jitinterface.cpp:10321 #8 0x0000006078a4bb18 in assertAbort (why=0x6078d355f9 "inArgMask & RBM_ARG_REGS", file=0x6078d355a0 "/home/clamp/Work/dotnet/riscv64/src/coreclr/jit/regalloc.cpp", line=133) at /home/clamp/Work/dotnet/riscv64/src/coreclr/jit/error.cpp:299 #9 0x0000006078a4c05e in noWayAssertBody (cond=0x0, file=0x45201 <error: Cannot access memory at address 0x45201>, line=6) at /home/clamp/Work/dotnet/riscv64/src/coreclr/jit/error.cpp:535 #10 0x0000006078a4b7b0 in noWayAssertBodyConditional (cond=0x6078d355f9 "inArgMask & RBM_ARG_REGS", file=0x6078d355a0 "/home/clamp/Work/dotnet/riscv64/src/coreclr/jit/regalloc.cpp", line=133) at /home/clamp/Work/dotnet/riscv64/src/coreclr/jit/error.cpp:520 #11 0x0000006078be1ee4 in Compiler::raUpdateRegStateForArg (this=<optimized out>, regState=0x400029ee00, argDsc=0x400029f608) at /home/clamp/Work/dotnet/riscv64/src/coreclr/jit/regalloc.cpp:133 #12 0x0000006078b7b966 in LinearScan::buildIntervals<true> (this=0x400025e828) at /home/clamp/Work/dotnet/riscv64/src/coreclr/jit/lsrabuild.cpp:2275 #13 0x0000006078b57a70 in LinearScan::doLinearScan (this=0x400025e828) at /home/clamp/Work/dotnet/riscv64/src/coreclr/jit/lsra.cpp:1374 #14 0x0000006078a36392 in Compiler::compCompile(void**, unsigned int*, JitFlags*)::$_2::operator()() const (this=<optimized out>) at /home/clamp/Work/dotnet/riscv64/src/coreclr/jit/compiler.cpp:5126 #15 ActionPhase<Compiler::compCompile(void**, unsigned int*, JitFlags*)::$_2>::DoPhase() (this=<optimized out>) at /home/clamp/Work/dotnet/riscv64/src/coreclr/jit/phase.h:64 #16 0x0000006078bc7116 in Phase::Run (this=0x6079a4f860) at /home/clamp/Work/dotnet/riscv64/src/coreclr/jit/phase.cpp:61 #17 0x0000006078a2d816 in DoPhase<Compiler::compCompile(void**, unsigned int*, JitFlags*)::$_2>(Compiler*, Phases, Compiler::compCompile(void**, unsigned int*, JitFlags*)::$_2) (_compiler=<optimized out>, _phase=<optimized out>, _action=...) at /home/clamp/Work/dotnet/riscv64/src/coreclr/jit/phase.h:78 #18 Compiler::compCompile (this=<optimized out>, methodCodePtr=<optimized out>, methodCodeSize=<optimized out>, compileFlags=<optimized out>) at /home/clamp/Work/dotnet/riscv64/src/coreclr/jit/compiler.cpp:5127 #19 0x0000006078a30bda in Compiler::compCompileHelper (this=0x400029ddd8, classPtr=0x3f843aa978, compHnd=0x6079a50610, methodInfo=0x6079a50478, methodCodePtr=0x6079a502d8, methodCodeSize=0x6079a50454, compileFlags=0x6079a502f8) at /home/clamp/Work/dotnet/riscv64/src/coreclr/jit/compiler.cpp:7061 #20 0x0000006078a2f482 in Compiler::compCompile(CORINFO_MODULE_STRUCT_*, void**, unsigned int*, JitFlags*)::$_3::operator()(Compiler::compCompile(CORINFO_MODULE_STRUCT_*, void**, unsigned int*, JitFlags*)::__JITParam*) const ( this=<optimized out>, __JITpParam=<optimized out>) at /home/clamp/Work/dotnet/riscv64/src/coreclr/jit/compiler.cpp:6217 #21 Compiler::compCompile (this=0x400029ddd8, classPtr=0x3f843aa978, methodCodePtr=0x6079a502d8, methodCodeSize=0x6079a50454, compileFlags=0x6079a502f8) at /home/clamp/Work/dotnet/riscv64/src/coreclr/jit/compiler.cpp:6236 #22 0x0000006078a31bae in jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::$_5::operator()(jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::__JITParam*) const::{lambda(jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::$_5::operator()(jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::__JITParam*) const::__JITParam*)#1}::operator()(jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::$_5::operator()(jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::__JITParam*) const::__JITParam*) const (this=<optimized out>, __JITpParam=<optimized out>) at /home/clamp/Work/dotnet/riscv64/src/coreclr/jit/compiler.cpp:7707 #23 jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::$_5::operator()(jitNativeCode(CORINFO_METHOD_STRUCT_*, CORINFO_MODULE_STRUCT_*, ICorJitInfo*, CORINFO_METHOD_INFO*, void**, unsigned int*, JitFlags*, void*)::__JITParam*) const (__JITpParam=<optimized out>, this=<optimized out>) at /home/clamp/Work/dotnet/riscv64/src/coreclr/jit/compiler.cpp:7732 #24 jitNativeCode (methodHnd=0x3f84403160, classPtr=0x3f843aa978, compHnd=0x6079a50610, methodInfo=0x6079a50478, methodCodePtr=0x6079a502d8, methodCodeSize=0x6079a50454, compileFlags=0x6079a502f8, inlineInfoPtr=0x0) at /home/clamp/Work/dotnet/riscv64/src/coreclr/jit/compiler.cpp:7734 #25 0x0000006078a3b5ea in CILJit::compileMethod (this=<optimized out>, compHnd=0x6079a50610, methodInfo=0x6079a50478, flags=<optimized out>, entryAddress=0x6079a50fa0, nativeSizeOfCode=0x6079a50454) at /home/clamp/Work/dotnet/riscv64/src/coreclr/jit/ee_il_dll.cpp:269 #26 0x0000004002d65d4e in invokeCompileMethodHelper (jitMgr=0x400008fe70, comp=0x6079a50610, info=0x6079a50478, jitFlags=..., nativeEntry=0x6079a50fa0, nativeSizeOfCode=0x6079a50454) at /home/clamp/Work/dotnet/riscv64/src/coreclr/vm/jitinterface.cpp:12206 #27 0x0000004002d65ec0 in invokeCompileMethod (jitMgr=0x0, comp=0x45201, info=0x6, jitFlags=..., nativeEntry=0x1, nativeSizeOfCode=0x6079a52160) at /home/clamp/Work/dotnet/riscv64/src/coreclr/vm/jitinterface.cpp:12269 #28 0x0000004002d66fac in UnsafeJitFunction (config=<optimized out>, ILHeader=<optimized out>, pJitFlags=0x6079a510f0, pSizeOfCode=0x6079a51184) at /home/clamp/Work/dotnet/riscv64/src/coreclr/vm/jitinterface.cpp:12714 #29 0x0000004002db4a66 in MethodDesc::JitCompileCodeLocked (this=0x3f84403160, pConfig=0x6079a513c8, pEntry=0x607c074f20, pSizeOfCode=0x6079a51184) at /home/clamp/Work/dotnet/riscv64/src/coreclr/vm/prestub.cpp:930 #30 0x0000004002db407e in MethodDesc::JitCompileCodeLockedEventWrapper (this=0x3f84403160, pConfig=0x6079a513c8, pEntry=0x607c074f20) at /home/clamp/Work/dotnet/riscv64/src/coreclr/vm/prestub.cpp:757 #31 0x0000004002db38c2 in MethodDesc::JitCompileCode (this=0x3f84403160, pConfig=0x6079a513c8) at /home/clamp/Work/dotnet/riscv64/src/coreclr/vm/prestub.cpp:698 #32 0x0000004002db2f9a in MethodDesc::PrepareILBasedCode (this=0x3f84403160, pConfig=0x6079a513c8) at /home/clamp/Work/dotnet/riscv64/src/coreclr/vm/prestub.cpp:424 #33 0x0000004002df6ed8 in TieredCompilationManager::CompileCodeVersion (this=<optimized out>, nativeCodeVersion=...) at /home/clamp/Work/dotnet/riscv64/src/coreclr/vm/tieredcompilation.cpp:961 #34 0x0000004002df6014 in TieredCompilationManager::OptimizeMethod (this=0x400008d648, nativeCodeVersion=...) at /home/clamp/Work/dotnet/riscv64/src/coreclr/vm/tieredcompilation.cpp:938 #35 TieredCompilationManager::DoBackgroundWork (this=0x400008d648, workDurationTicksRef=0x6079a515a0, minWorkDurationTicks=12000000, maxWorkDurationTicks=50000000) at /home/clamp/Work/dotnet/riscv64/src/coreclr/vm/tieredcompilation.cpp:823 #36 0x0000004002df52c6 in TieredCompilationManager::BackgroundWorkerStart (this=0x400008d648) at /home/clamp/Work/dotnet/riscv64/src/coreclr/vm/tieredcompilation.cpp:537 #37 0x0000004002df50f0 in TieredCompilationManager::BackgroundWorkerBootstrapper1 () at /home/clamp/Work/dotnet/riscv64/src/coreclr/vm/tieredcompilation.cpp:485 #38 0x0000004002defc54 in ManagedThreadBase_DispatchInner (pCallState=0x6079a517b0) at /home/clamp/Work/dotnet/riscv64/src/coreclr/vm/threads.cpp:7222 #39 ManagedThreadBase_DispatchMiddle (pCallState=0x6079a517b0) at /home/clamp/Work/dotnet/riscv64/src/coreclr/vm/threads.cpp:7266 #40 ManagedThreadBase_DispatchOuter(ManagedThreadCallState*)::$_7::operator()(ManagedThreadBase_DispatchOuter(ManagedThreadCallState*)::TryArgs*) const::{lambda(Param*)#1}::operator()(Param*) const (pParam=<optimized out>, this=<optimized out>) at /home/clamp/Work/dotnet/riscv64/src/coreclr/vm/threads.cpp:7424 #41 ManagedThreadBase_DispatchOuter(ManagedThreadCallState*)::$_7::operator()(ManagedThreadBase_DispatchOuter(ManagedThreadCallState*)::TryArgs*) const (this=<optimized out>, pArgs=<optimized out>) at /home/clamp/Work/dotnet/riscv64/src/coreclr/vm/threads.cpp:7426 #42 ManagedThreadBase_DispatchOuter (pCallState=<optimized out>) at /home/clamp/Work/dotnet/riscv64/src/coreclr/vm/threads.cpp:7450 #43 0x0000004002df02ca in ManagedThreadBase_FullTransition (pTarget=<optimized out>, args=0x45201, filterType=ManagedThread) at /home/clamp/Work/dotnet/riscv64/src/coreclr/vm/threads.cpp:7470 #44 ManagedThreadBase::KickOff (pTarget=<optimized out>, args=<optimized out>) at /home/clamp/Work/dotnet/riscv64/src/coreclr/vm/threads.cpp:7505 #45 0x0000004002df502e in TieredCompilationManager::BackgroundWorkerBootstrapper0 (args=<optimized out>) at /home/clamp/Work/dotnet/riscv64/src/coreclr/vm/tieredcompilation.cpp:466 #46 0x0000004003250222 in CorUnix::CPalThread::ThreadEntry (pvParam=0x40003676b0) at /home/clamp/Work/dotnet/riscv64/src/coreclr/pal/src/thread/thread.cpp:1760 #47 0x0000004002b4c56e in ?? () from /usr/riscv64-linux-gnu/lib/libc.so.6

I think the struct which is passed by Split should not be promoted.
Early the CanPromoteStructType should be false?

@shushanhf Sure. I will fix for both LA64 and RISCV64 like what you mentioned. Thank you!

I updated. Please leave any comment if you have any question and request. Thank you.

jakobbotsch · 2023-06-21T09:06:19Z

src/coreclr/jit/lclvars.cpp

+#if defined(TARGET_LOONGARCH64) || defined(TARGET_RISCV64)
+    if (varDsc->lvIsSplit)
+    {
+        JITDUMP("  struct promotion of V%02u is disabled because it is splitted\n", lclNum);
+        return false;
+    }
+#endif // TARGET_LOONGARCH64 || TARGET_RISCV64


ARM32 has split parameters and does not need this so I do not think this is the right fix.

One thing ARM32 does is to always use dependent promotion for parameters. Can you please check if making lvaGetPromotionType return PROMOTION_TYPE_DEPENDENT for split parameters is enough to fix the problem?

If not then we should understand why and how ARM32 differs in its handling so that we can align the behavior of all 3 platforms.

The LoongArch64 is OK for the above two cases, this issue doesn't meet on LA64.

This is strange.
I will also debug it on LA64.

One thing ARM32 does is to always use dependent promotion for parameters. Can you please check if making lvaGetPromotionType return PROMOTION_TYPE_DEPENDENT for split parameters is enough to fix the problem?

I checked however it cannot fix the problem.

The LoongArch64 is OK for the above two cases, this issue doesn't meet on LA64.

Thank you so much for sharing the test results on LA64 CHECKED build. I think there are some issues in RISC-V only. Something is missed or implemented in wrong way in RISC-V only.

I will investigate the split parameters of ARM32 and RISC-V. If you have any comments about the problem, please let me know. It will be very helpful to me.
Thank you.

We tested these cases some times.
The console format is ok, but the cases tested by new xunit will failed random on runtime8.0 while runtime6.0 is ok.

As the Dragon Boat Festival for Chinese, I will debug it on sunday.

I found a difference between ARM32 and RISCV64.
ARM32 doesn't support FEATURE_MULTIREG_STRUCT_PROMOTE. So in ARM32, ShouldPromoteStructVar returns false with Not promoting promotable struct local V03, because lvIsParam is true and #fields = 2 message for the split parameters.

runtime/src/coreclr/jit/lclvars.cpp

Lines 2158 to 2194 in 2e764d6

else if (varDsc->lvIsParam && !compiler->lvaIsImplicitByRefLocal(lclNum) && !varDsc->lvIsHfa())

{

#if FEATURE_MULTIREG_STRUCT_PROMOTE

// Is this a variable holding a value with exactly two fields passed in

// multiple registers?

if (compiler->lvaIsMultiregStruct(varDsc, compiler->info.compIsVarArgs))

{

if (structPromotionInfo.containsHoles && structPromotionInfo.customLayout)

{

JITDUMP("Not promoting multi-reg struct local V%02u with holes.\n", lclNum);

shouldPromote = false;

}

else if ((structPromotionInfo.fieldCnt != 2) &&

!((structPromotionInfo.fieldCnt == 1) && varTypeIsSIMD(structPromotionInfo.fields[0].fldType)))

{

JITDUMP("Not promoting multireg struct local V%02u, because lvIsParam is true, #fields != 2 and it's "

"not a single SIMD.\n",

lclNum);

shouldPromote = false;

}

}

else

#endif // !FEATURE_MULTIREG_STRUCT_PROMOTE

// TODO-PERF - Implement struct promotion for incoming single-register structs.

// Also the implementation of jmp uses the 4 byte move to store

// byte parameters to the stack, so that if we have a byte field

// with something else occupying the same 4-byte slot, it will

// overwrite other fields.

if (structPromotionInfo.fieldCnt != 1)

{

JITDUMP("Not promoting promotable struct local V%02u, because lvIsParam is true and #fields = "

"%d.\n",

lclNum, structPromotionInfo.fieldCnt);

shouldPromote = false;

}

}

This is my test codes with RISCV64 and ARM32.
Build as CHECKED and executed with export COMPlus_TieredCompilation=0
In RISCV64, it fails with the error message.
In ARM32, it passed with the jit dump message Not promoting promotable struct local V03, because lvIsParam is true and #fields = 2.

namespace MyApp { internal ref struct Test { [MethodImplAttribute(MethodImplOptions.NoInlining)] private Test(int a, int b, int c, int d, int e, int f, Span<int> g, int h) // FOR RISCV64 { Console.WriteLine(a); Console.WriteLine(b); Console.WriteLine(c); Console.WriteLine(d); Console.WriteLine(e); Console.WriteLine(f); Console.WriteLine(g.IsEmpty); Console.WriteLine(h); } [MethodImplAttribute(MethodImplOptions.NoInlining)] private Test(int a, int b, Span<int> c, int d) // FOR ARM32 { Console.WriteLine(a); Console.WriteLine(b); Console.WriteLine(c.IsEmpty); Console.WriteLine(d); } [MethodImplAttribute(MethodImplOptions.NoInlining)] public static void check() { var a = new Test(1, 2, 3, 4, 5, 6, stackalloc int[2], 9); var b = new Test(1, 2, stackalloc int[2], 3); } } internal class Program { static void Main(string[] args) { Test.check(); return; } } }

I see. Then the current fix seems fine to me -- but maybe move it into the FEATURE_MULTIREG_STRUCT_PROMOTE section you have highlighted?

Updated. Thank you.

clamp03 · 2023-06-22T12:49:42Z

@shushanhf Please review next week after your festival. :) Thank you!

shushanhf · 2023-06-25T10:22:29Z

@shushanhf Please review next week after your festival. :) Thank you!

Thanks
I agree with this patch.

[RISC-V][JIT] Reset lvIsRegArg in splitted arg

aeef514

ghost added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI community-contribution Indicates that the PR has been added by a community member labels Jun 21, 2023

clamp03 self-assigned this Jun 21, 2023

clamp03 added the arch-riscv Related to the RISC-V architecture label Jun 21, 2023

Remove LA

ffc0b71

shushanhf reviewed Jun 21, 2023

View reviewed changes

Updated based on the review

0e63e89

clamp03 changed the title ~~[RISC-V][JIT] Reset lvIsRegArg in splitted arg~~ [RISC-V][JIT] Disable promotion for splitted args Jun 21, 2023

jakobbotsch reviewed Jun 21, 2023

View reviewed changes

clamp03 marked this pull request as draft June 21, 2023 12:29

Moved codes

9d2c249

clamp03 marked this pull request as ready for review June 22, 2023 12:45

jakobbotsch approved these changes Jun 22, 2023

View reviewed changes

t-mustafin approved these changes Jun 22, 2023

View reviewed changes

jakobbotsch merged commit 0633ecf into dotnet:main Jun 25, 2023

clamp03 deleted the fix branch June 25, 2023 23:13

ghost locked as resolved and limited conversation to collaborators Jul 26, 2023

	else if (varDsc->lvIsParam && !compiler->lvaIsImplicitByRefLocal(lclNum) && !varDsc->lvIsHfa())
	{
	#if FEATURE_MULTIREG_STRUCT_PROMOTE
	// Is this a variable holding a value with exactly two fields passed in
	// multiple registers?
	if (compiler->lvaIsMultiregStruct(varDsc, compiler->info.compIsVarArgs))
	{
	if (structPromotionInfo.containsHoles && structPromotionInfo.customLayout)
	{
	JITDUMP("Not promoting multi-reg struct local V%02u with holes.\n", lclNum);
	shouldPromote = false;
	}
	else if ((structPromotionInfo.fieldCnt != 2) &&
	!((structPromotionInfo.fieldCnt == 1) && varTypeIsSIMD(structPromotionInfo.fields[0].fldType)))
	{
	JITDUMP("Not promoting multireg struct local V%02u, because lvIsParam is true, #fields != 2 and it's "
	"not a single SIMD.\n",
	lclNum);
	shouldPromote = false;
	}
	}
	else
	#endif // !FEATURE_MULTIREG_STRUCT_PROMOTE

	// TODO-PERF - Implement struct promotion for incoming single-register structs.
	// Also the implementation of jmp uses the 4 byte move to store
	// byte parameters to the stack, so that if we have a byte field
	// with something else occupying the same 4-byte slot, it will
	// overwrite other fields.
	if (structPromotionInfo.fieldCnt != 1)
	{
	JITDUMP("Not promoting promotable struct local V%02u, because lvIsParam is true and #fields = "
	"%d.\n",
	lclNum, structPromotionInfo.fieldCnt);
	shouldPromote = false;
	}
	}

Conversation

clamp03 commented Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 21, 2023

Uh oh!

clamp03 commented Jun 21, 2023

Uh oh!

shushanhf commented Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clamp03 commented Jun 21, 2023

Uh oh!

shushanhf Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clamp03 Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shushanhf Jun 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clamp03 Jun 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clamp03 commented Jun 22, 2023

Uh oh!

shushanhf commented Jun 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

clamp03 commented Jun 21, 2023 •

edited

Loading

shushanhf commented Jun 21, 2023 •

edited

Loading

shushanhf Jun 21, 2023 •

edited

Loading

clamp03 Jun 21, 2023 •

edited

Loading

shushanhf Jun 21, 2023 •

edited

Loading

clamp03 Jun 22, 2023 •

edited

Loading