[RISC-V] Add crossgen2 for riscv64 #95188

ashaurtaev · 2023-11-23T22:41:06Z

Added crossgen2 for riscv64

Part of #84834
cc @wscho77 @HJLeee @clamp03 @JongHeonChoi @t-mustafin @gbalykov @yurai007 @sirntar @tomeksowi @Bajtazar @brucehoult

ashaurtaev · 2023-11-23T22:44:41Z

@clamp03 @gbalykov Currently, compilation of the r2r image and work with return 0 have been added. In other cases, a segfault occurs

clamp03 · 2023-11-23T22:55:29Z

Please update casing from Riscv64 to RiscV64 in all names according to #73437 (comment)

clamp03 · 2023-11-23T22:56:20Z

src/coreclr/tools/Common/CommandLineHelpers.cs

@@ -96,6 +96,7 @@ public static TargetArchitecture GetTargetArchitecture(string token)
                    Architecture.Arm => TargetArchitecture.ARM,
                    Architecture.Arm64 => TargetArchitecture.ARM64,
                    Architecture.LoongArch64 => TargetArchitecture.LoongArch64,
+                    (Architecture)9 => TargetArchitecture.Riscv64,


I think Architecture.RiscV64 is supported from #90203.

We'll have to wait for new architecture to get to sdk and for this new sdk to be used in main branch build. Currently .net-8.0.1 sdk is used for main branch, so it'll take some time until .net9-preview1 sdk arrives (changes from #90203 are not present in .net8 branch). Maybe I'm wrong here, if so please correct me.

My mistake. Thank you!

clamp03 · 2023-11-24T08:32:00Z

...t/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/Target_Riscv64/ImportThunk.cs

+                    {
+                        // ori a4, r0, #index
+                        int index = _containingImportSection.IndexFromBeginningOfArray;
+                        instructionEncoder.EmitMOV(Register.X14, checked((ushort)index));


Why do you use X14? Isn't T0 (X5) correct?

clamp03 · 2023-11-24T08:32:15Z

...t/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/Target_Riscv64/ImportThunk.cs

+
+                    if (!relocsOnly)
+                    {
+                        // ori a4, r0, #index


update ori to addi and update a4 to T0

clamp03 · 2023-11-24T08:32:50Z

...t/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/Target_Riscv64/ImportThunk.cs

+
+                case Kind.DelayLoadHelper:
+                case Kind.VirtualStubDispatch:
+                    // T8 contains indirection cell


There is no T8 in RISCV. Please update to T5.

clamp03 · 2023-11-24T08:33:33Z

src/coreclr/tools/Common/Compiler/DependencyAnalysis/Relocation.cs

+
+            // then get the low 12 bits
+            pcInstr = *(pCode + 1);
+            imm += (long)(pcInstr & 0xFFFF);


Update to 0xFFF with correct shifts for JALR.

clamp03 · 2023-11-24T08:34:38Z

src/coreclr/tools/Common/Compiler/DependencyAnalysis/ObjectDataBuilder.cs

+                case RelocType.IMAGE_REL_BASED_RISCV64_PC:
+                case RelocType.IMAGE_REL_BASED_RISCV64_JALR:


I think IMAGE_REL_BASED_RISCV64_PC and IMAGE_REL_BASED_RISCV64_JALR has the same behavior in RISCV64. IMO, it is better to merge into one such as IMAGE_REL_BASED_RISCV64_AUIPC You need to update IMAGE_REL_RISCV64_PC and IMAGE_REL_RISCV64_JALR in runtime as well.

clamp03 · 2023-11-24T08:36:45Z

src/coreclr/tools/Common/Compiler/DependencyAnalysis/Relocation.cs

+            Debug.Assert((imm20 & 0x1) == 0);    // the low two bits must be zero
+
+            uint auipcInstr = *pCode;
+            Debug.Assert(auipcInstr == 0x00000297);    // must be auipc t0, 0


Why must it be auipc t0, 0?

removed incorrect comment

clamp03 · 2023-11-24T08:37:41Z

src/coreclr/tools/aot/ILCompiler.ReadyToRun/ObjectWriter/SectionBuilder.cs

@@ -317,6 +317,10 @@ public SectionBuilder(TargetDetails target)
                    _codePadding = 0x002A0005u;
                    break;

+                case TargetArchitecture.Riscv64:
+                    _codePadding = 0x002A0005u;


Could you explain why it is 0x002A0005u? In other archs, it is break instruction. Please clarify.

clamp03 · 2023-11-24T08:38:30Z

src/coreclr/tools/Common/Compiler/DependencyAnalysis/Target_Riscv64/Riscv64Emitter.cs

+            else
+            {
+                //Builder.EmitReloc(symbol, RelocType.IMAGE_REL_BASED_RISCV64_PC);
+                Builder.EmitUInt(0xffffffff); // bad code.


0x00000000 is used for bad code.

clamp03 · 2023-11-24T08:39:45Z

src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/TransitionBlock.cs

+
+            public override int PointerSize =>  8;
+            public override int NumArgumentRegisters => 8;
+            public override int NumCalleeSavedRegisters => 12;


Is NumCalleeSavedRegisters correct number? Please write the comments like other architectures.

clamp03 · 2023-11-24T08:53:56Z

src/coreclr/tools/Common/Compiler/DependencyAnalysis/Relocation.cs

+            uint auipcInstr = *pCode;
+
+            // first get the high 20 bits,
+            int imm = (int)(((auipcInstr >> 12) & 0xFFFFF) << 12);


I think it is okay like auipcInstr & 0xFFFFF000 (no shifts)

clamp03 · 2023-11-24T08:54:40Z

src/coreclr/tools/Common/Compiler/DependencyAnalysis/Relocation.cs

+
+            // then get the low 12 bits,
+            auipcInstr = *(pCode + 1);
+            imm += ((int)(((auipcInstr >> 20) & 0xFFF) << 20)) >> 20;


I think ((int)(auipcInstr & 0xfff00000)) >> 20 is better.

clamp03 · 2023-11-24T08:59:15Z

src/coreclr/tools/Common/Compiler/DependencyAnalysis/Relocation.cs

+
+            int relOff = (int)imm32 & 0x800;
+            int imm = (int)imm32 + relOff;
+            relOff = ((imm & 0x7ff) - relOff) & 0xfff;


Isn't it good to make codes similar to codes in jit/emitriscv64.cpp?

runtime/src/coreclr/jit/emitriscv64.cpp

Lines 2423 to 2433 in cc4db9b

code = 0x00000017;

*(code_t*)dstRW = code | (code_t)reg1 << 7 | ((imm + 0x800) & 0xfffff000);

dstRW += 4;

#ifdef DEBUG

code = emitInsCode(INS_auipc);

assert(code == 0x00000017);

code = emitInsCode(INS_addi);

assert(code == 0x00000013);

#endif

ins = INS_addi;

*(code_t*)dstRW = 0x00000013 | ((code_t)reg1 << 7) | ((code_t)reg1 << 15) | ((doff & 0xfff) << 20);

clamp03 · 2023-11-24T09:02:29Z

Please add commits not force-push when you fixed.

clamp03 · 2023-11-24T09:21:59Z

src/coreclr/tools/Common/Compiler/DependencyAnalysis/Target_Riscv64/Riscv64Emitter.cs

+                // ld a0, 16(t0)
+                EmitLD(Register.X10, Register.X5, 16);
+                // ld t0, 24(t0)
+                EmitLD(Register.X5, Register.X5, 24);


Could you explain why 24? It seems like 0 to me.

clamp03 · 2023-11-24T09:53:52Z

src/coreclr/tools/Common/Compiler/DependencyAnalysis/Target_Riscv64/Riscv64Emitter.cs

+                // auipc t0, 0
+                EmitPC(Register.X5);
+                // ld a0, 16(t0)
+                EmitLD(Register.X10, Register.X5, 16);


I think EmitJMP jumps to symbol with relocation support. But, it is different. Could you explain why you generate these code?

clamp03 · 2023-11-24T10:22:25Z

src/coreclr/tools/Common/Compiler/DependencyAnalysis/Target_Riscv64/Riscv64Emitter.cs

+            uint offset = symbol.RepresentsIndirectionCell ? 7u : 2u;
+
+            // bne regSrc, x0, offset
+            Builder.EmitUInt((uint)(0x00001063 |  ((uint)regSrc << 15) | (offset << 31)));


Offset is instructions to jump. So offset value in instructions should be 7 * 4(inst size) or 2 * 4. So shift for offset is not 31 at all. Maybe 9? Please check.

clamp03 · 2023-11-27T00:56:53Z

...t/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/Target_Riscv64/ImportThunk.cs

+                    instructionEncoder.EmitPC(Register.X6);
+
+                    // load Module* -> T1
+                    instructionEncoder.EmitLD(Register.X6, Register.X6, 0x18);


I think it is wrong. Please update to correct offset.

clamp03 · 2023-11-27T00:56:59Z

...t/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/Target_Riscv64/ImportThunk.cs

+                    instructionEncoder.EmitPC(Register.X11);
+
+                    // load Module* -> a1
+                    instructionEncoder.EmitLD(Register.X11, Register.X11, 0x18);


tomeksowi · 2023-11-24T17:41:45Z

src/coreclr/tools/Common/Compiler/DependencyAnalysis/Relocation.cs

+            // Verify that we got a valid offset
+            Debug.Assert((imm20 >= -0x80000) && (imm20 < 0x80000));
+
+            Debug.Assert((imm20 & 0x1) == 0);    // the low two bits must be zero


You're checking only the lowest bit, which afaik is correct. Pls update comment.

tomeksowi · 2023-11-24T17:50:07Z

src/coreclr/tools/Common/Compiler/DependencyAnalysis/Target_Riscv64/Riscv64Emitter.cs

+        public void EmitMOV(Register regDst, ushort imm16)
+        {
+            Debug.Assert((uint)regDst <= 0x1f);
+            Debug.Assert(imm16 <= 0xfff);


Nit: so it's imm12.

tomeksowi · 2023-11-24T17:54:15Z

src/coreclr/tools/Common/Compiler/DependencyAnalysis/Target_Riscv64/Riscv64Emitter.cs

+            Builder.EmitUInt(0x00100073);
+        }
+
+        public void EmitMOV(Register regDst, ushort imm16)


Nit: more like EmitLI (load immediate), mov means register move on RISC-V.

tomeksowi · 2023-11-24T18:03:40Z

src/coreclr/tools/Common/Compiler/DependencyAnalysis/Target_Riscv64/Riscv64Emitter.cs

+        public void EmitRETIfEqual(Register regSrc)
+        {
+            // bne regSrc, x0, 8
+            Builder.EmitUInt((uint)(0x00001463 | ((uint)regSrc << 15)));
+            EmitRET();
+        }


Nit: "equal" suggests comparing with sth else. Maybe rename to EmitRETIfZero?

tomeksowi · 2023-11-24T18:36:52Z

...er.Compiler/Compiler/DependencyAnalysis/Target_Riscv64/Riscv64ReadyToRunGenericHelperNode.cs

+                            // We need to trigger the cctor before returning the base. It is stored at the beginning of the non-GC statics region.
+                            encoder.EmitADD(encoder.TargetRegister.Arg3, encoder.TargetRegister.Arg0, -NonGCStaticsNode.GetClassConstructorContextSize(factory.Target));
+                            encoder.EmitLD(encoder.TargetRegister.Arg2, encoder.TargetRegister.Arg3, 0);
+                            encoder.EmitXOR(encoder.TargetRegister.IntraProcedureCallScratch1, encoder.TargetRegister.Arg2, 0);


This is an oddly phrased register move (because x ^ 0 == x). Did you mean x ^ 1 like in the rest?

(if you really meant a reg move, 1. it should be addi, 2. IMO it can be elided, the branch below could work on the source register)

tomeksowi · 2023-11-24T18:57:43Z

src/coreclr/tools/Common/Compiler/DependencyAnalysis/Relocation.cs

+            auipcInstr = *(pCode + 1);
+
+            // Assemble the pc-relative low 12 bits of 'imm20' into the addi
+            auipcInstr |= (uint)(relOff << 20);


Nit: it's no longer an auipc. Pls call it e.g. addiInstr and assert it really is an addi.

tomeksowi · 2023-11-24T19:08:26Z

...er.Compiler/Compiler/DependencyAnalysis/Target_Riscv64/Riscv64ReadyToRunGenericHelperNode.cs

+
+                            encoder.EmitADD(encoder.TargetRegister.Arg2, encoder.TargetRegister.Arg2, -NonGCStaticsNode.GetClassConstructorContextSize(factory.Target));
+                            encoder.EmitLD(encoder.TargetRegister.Arg3, encoder.TargetRegister.Arg2, factory.Target.PointerSize);
+                            encoder.EmitXOR(encoder.TargetRegister.IntraProcedureCallScratch1, encoder.TargetRegister.Arg3, 1);


Could you explain why are you flipping the lowest bit here?

tomeksowi · 2023-11-25T09:58:44Z

...t/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/Target_Riscv64/ImportThunk.cs

+                    // load Module* -> T1
+                    instructionEncoder.EmitLD(Register.X6, Register.X6, 0x18);
+
+                    // ld_d X6, X6, 0


Suggested change

// ld_d X6, X6, 0

// ld X6, X6, 0

tomeksowi · 2023-11-25T10:24:33Z

src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/Riscv64/Registers.cs

+{
+    public enum Registers
+    {
+        R0,


Let's adhere to naming in RISC-V ABI.

Suggested change

R0,

Zero,

tomeksowi · 2023-11-26T18:42:36Z

...oreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ArgIterator.cs

+                            Debug.Assert((cbArg % _transitionBlock.PointerSize) == 0);
+
+                            int regSlots = ALIGN_UP(cbArg, _transitionBlock.PointerSize) / _transitionBlock.PointerSize;
+                            // Only R4-R11 are valid argument registers.


More like x10-x17 on RISC-V

sirntar · 2023-11-27T08:29:17Z

src/coreclr/tools/Common/Compiler/DependencyAnalysis/Target_Riscv64/Register.cs

+        X0 = 0,
+        X1 = 1,
+        X2 = 2,
+        X3 = 3,
+        X4 = 4,
+        X5 = 5,
+        X6 = 6,
+        X7 = 7,
+        X8 = 8,
+        X9 = 9,
+        X10 = 10,
+        X11 = 11,
+        X12 = 12,
+        X13 = 13,
+        X14 = 14,
+        X15 = 15,
+        X16 = 16,
+        X17 = 17,
+        X18 = 18,
+        X19 = 19,
+        X20 = 20,
+        X21 = 21,
+        X22 = 22,
+        X23 = 23,
+        X24 = 24,
+        X25 = 25,
+        X26 = 26,
+        X27 = 27,
+        X28 = 28,
+        X29 = 29,
+        X30 = 30,
+
+        X31 = 31,


Any reason for not to use RISC-V ABI registers, that are more readable?

sirntar · 2023-11-27T08:45:04Z

src/coreclr/tools/Common/Compiler/DependencyAnalysis/Target_Riscv64/TargetRegisterMap.cs

+            Arg0 = Register.X10;
+            Arg1 = Register.X11;
+            Arg2 = Register.X12;
+            Arg3 = Register.X13;
+            Arg4 = Register.X14;
+            Arg5 = Register.X15;
+            Arg6 = Register.X16;
+            Arg7 = Register.X17;
+            IntraProcedureCallScratch1 = Register.X28;
+            Result = Register.X10;


If your enum Register would be compatible with ABI, then here you wouldn't have less readable Register.Xn, but instead:

Arg0 = Register.A0; Arg1 = Register.A1; Arg2 = Register.A2; Arg3 = Register.A3; Arg4 = Register.A4; Arg5 = Register.A5; Arg6 = Register.A6; Arg7 = Register.A7; IntraProcedureCallScratch1 = Register.T3; Result = Register.A0;

gbalykov · 2023-12-21T14:32:35Z

@sirntar @tomeksowi @clamp03

Please note that these are just initial changes required for crossgen2 and just simple functionality works (both r2r image generation and launch of image are successful with debug/release runtime):

console helloworld compiled in debug (build -c Debug)
parts of JIT CodeGenBringUp tests (160 out of 641)

In order to not extend this PR more, further work can be done in separate PRs (if you will see no issues in this code):

potentially rename Registers to ABI namings instead of default Xn ones (for now Registers is implemented in the same way as for other architectures for consistency)
there might be issues/NYI with release dlls, disabled W^X (DOTNET_EnableWriteXorExecute=0) or some other non-default configs
potentially remove IMAGE_REL_BASED_RISCV64_JALR from runtime (removed just from crossgen2 for now)
R2RDump porting to RISC-V (few R2RDump-related changes are removed from this PR)
increase amount of passed tests
etc.

ashaurtaev · 2023-12-21T14:56:01Z

Thanks a lot for your reviews, they were very useful! @clamp03 @tomeksowi @sirntar

clamp03 · 2023-12-21T23:46:32Z

src/coreclr/tools/Common/JitInterface/CorInfoImpl.cs


                    switch (fRelocType)
                    {
                        case IMAGE_REL_RISCV64_PC:
                            return RelocType.IMAGE_REL_BASED_RISCV64_PC;
-                        case IMAGE_REL_RISCV64_JALR:
-                            return RelocType.IMAGE_REL_BASED_RISCV64_JALR;


IMO, these code be updated or removed as well. Please check.

runtime/src/coreclr/jit/emitriscv64.cpp

Line 1546 in 0cf461b

emitRecordRelocation(dst - 4, (BYTE*)addr, IMAGE_REL_RISCV64_JALR);

runtime/src/coreclr/pal/inc/rt/ntimage.h

Line 1028 in 0cf461b

#define IMAGE_REL_RISCV64_JALR 0x0004

I missed the comment. Please update in the next PR. Thank you.

JongHeonChoi · 2023-12-22T03:51:39Z

src/coreclr/tools/Common/Compiler/DependencyAnalysis/Relocation.cs

@@ -411,6 +412,47 @@ private static unsafe void PutLoongArch64JIR(uint* pCode, long imm38)
            Debug.Assert(GetLoongArch64JIR(pCode) == imm38);
        }

+        private static unsafe int GetRiscV64PC12(uint* pCode)


As I know, PC12 means pcaddu12i in LOONGARCH64. Please update the name to GetRiscV64AUIPC or GetRiscV64PC, Please remove 12 at least.

Suggested change

private static unsafe int GetRiscV64PC12(uint* pCode)

private static unsafe int GetRiscV64PC(uint* pCode)

Thanks, I’ll fix this

JongHeonChoi · 2023-12-22T03:51:43Z

src/coreclr/tools/Common/Compiler/DependencyAnalysis/ObjectDataBuilder.cs

@@ -319,6 +319,9 @@ public void EmitReloc(ISymbolNode symbol, RelocType relocType, int delta = 0)

                case RelocType.IMAGE_REL_BASED_LOONGARCH64_PC:
                case RelocType.IMAGE_REL_BASED_LOONGARCH64_JIR:
+
+                //TODO: consider removal of IMAGE_REL_BASED_RISCV64_JIR from runtime too


Please update IMAGE_REL_BASED_RISCV64_JIR to IMAGE_REL_RISCV64_JALR.
In the runtime, IMAGE_REL_RISCV64_JALR is used.

JongHeonChoi · 2023-12-22T03:52:11Z

src/coreclr/tools/Common/Compiler/DependencyAnalysis/Relocation.cs

+        //  case:EA_PTR_DSP_RELOC
+        //   auipc  reg, off-hi-20bits
+        //   ld     reg, reg, off-lo-12bits
+        private static unsafe void PutRiscV64PC12(uint* pCode, long imm32)


Update the name too.

Suggested change

private static unsafe void PutRiscV64PC12(uint* pCode, long imm32)

private static unsafe void PutRiscV64PC(uint* pCode, long imm32)

Thanks, I’ll fix this

JongHeonChoi · 2023-12-22T03:54:08Z

src/coreclr/tools/Common/Compiler/DependencyAnalysis/Target_RiscV64/RiscV64Emitter.cs

+            EmitRET();
+        }
+
+        public void EmitJE(Register regSrc, ISymbolNode symbol)


I think EmitJNEZ is better.

It's actually the same as for other arches meaning that we jump over the stuff generated by emitjmp in case condition is reversed (in this case ne)

I think EmitJE naming is for ARM* arch which use flag after cmp instruction. However, in RISC-V it doesn't compare and jump like ARM*. So I thought it is better if it shows RISC-V's code generation like what you changed EmitRetIfEqual to EmitRetIfZero.
I am confusing. I think EmitJEZ seems right.

This is just my suggestion. You can freely choose. :)

Ah, my bad, thought about the other thing, yes, you are right :) I meant that it's not EmitJNotfZero, but EmitJIfZero

JongHeonChoi · 2023-12-22T03:56:55Z

src/coreclr/tools/Common/Compiler/DependencyAnalysis/Target_RiscV64/RiscV64Emitter.cs

+            uint offset = symbol.RepresentsIndirectionCell ? 14u : 4u;
+            uint encodedOffset = ((offset & 0x1e) << 7) | ((offset & 0x7e0) << 20) | ((offset & 0x800) >> 4) | ((offset & 0x1000) << 19);


With your encodedOffset logic, offset should be 7 << 2 : 2 << 2. Please check again.

JongHeonChoi · 2023-12-22T03:57:23Z

src/coreclr/tools/Common/Compiler/DependencyAnalysis/Target_RiscV64/RiscV64Emitter.cs

+        }
+
+        public void EmitLI(Register regDst, ushort imm12)
+        {


Please add assert for imm12.

Thanks, I’ll fix this

JongHeonChoi · 2023-12-22T03:58:19Z

...ot/ILCompiler.Compiler/Compiler/DependencyAnalysis/Target_RiscV64/RiscV64UnboxingStubNode.cs

+        protected override void EmitCode(NodeFactory factory, ref RiscV64Emitter encoder, bool relocsOnly)
+        {
+            // addi a0, a0, sizeof(void*)
+            encoder.EmitADDI(encoder.TargetRegister.Arg0, encoder.TargetRegister.Arg0, factory.Target.PointerSize);


Nit: Update EmitADDI to EmitLI.

This is addition with a0, EmitLI does similar thing, but arg is x0

You are right! Thank you!

JongHeonChoi · 2023-12-22T03:59:22Z

...oreclr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/ArgIterator.cs

+        private int _riscv64IdxGenReg;  // Next general register to be assigned a value
+        private int _riscv64OfsStack;   // Offset of next stack location to be assigned a value
+        private int _riscv64IdxFPReg;   // Next FP register to be assigned a value


Adds more spaces for the comments like the others.

Thanks, I’ll fix this

JongHeonChoi · 2023-12-22T04:01:02Z

...lr/tools/aot/ILCompiler.ReadyToRun/Compiler/DependencyAnalysis/ReadyToRun/TransitionBlock.cs

+            // a0 .. a7
+            public override int NumArgumentRegisters => 8;
+            // fp=x8, ra=x1, s1-s11(R9,R18-R27), tp=x3, gp=x4
+            public override int NumCalleeSavedRegisters => 15;


In the comment, total fp, ra, s1-s11, tp and gp is 14. Could you check which is correct?

It seems that both are 15, or am I missing smth? x1, x3, x4, x8, x9, x18, x19, x20, x21, x22, x23, x24, x25, x26, x27

You are right! Thank you!

JongHeonChoi · 2023-12-22T04:01:25Z

src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/TransitionBlock.cs

+            // a0 .. a7
+            public override int NumArgumentRegisters => 8;
+            // fp=x8, ra=x1, s1-s11(R9,R18-R27), tp=x3, gp=x4
+            public override int NumCalleeSavedRegisters => 15;


In the comment, total fp, ra, s1-s11, tp and gp is 14. Could you check which is correct?

It seems that both are 15, or am I missing smth? x1, x3, x4, x8, x9, x18, x19, x20, x21, x22, x23, x24, x25, x26, x27

You are right! Thank you!

clamp03

LGTM Thanks for doing a great job with a difficult task!

JongHeonChoi

I applaud your great PR.

clamp03 · 2023-12-26T23:53:25Z

@dotnet/crossgen-contrib Can you please take a look at this pull request and provide feedback? Thank you.

davidwrighton

I've looked through the changes and they look good to me. In a few weeks I expect that we will have the updated .NET 9 sdk in place. Would you like to merge this change now and then fix the various TODO's around the Machine and Architecture enums once the new SDK is in place? If so, please file an issue to do so. Once that issue is in place, I'm willing to merge this PR.

davidwrighton · 2024-01-02T18:17:29Z

Also, we need a clean test results before I will merge, so please look into the failures/rerun stuff if necessary.

ivdiazsa · 2024-01-02T18:45:30Z

src/coreclr/tools/Common/CommandLineHelpers.cs

@@ -96,6 +96,7 @@ public static TargetArchitecture GetTargetArchitecture(string token)
                    Architecture.Arm => TargetArchitecture.ARM,
                    Architecture.Arm64 => TargetArchitecture.ARM64,
                    Architecture.LoongArch64 => TargetArchitecture.LoongArch64,
+                    (Architecture)9 => TargetArchitecture.RiscV64, /* TODO: update with Architecture.RiscV64 */


Is there a tracking work item for this TODO? It would be for the best to keep hard coded numbers like this one for as short as possible.

ivdiazsa · 2024-01-02T18:50:22Z

src/coreclr/tools/aot/ILCompiler.Diagnostics/ILCompiler.Diagnostics.csproj

@@ -6,7 +6,7 @@
    <IsDotNetFrameworkProductAssembly>true</IsDotNetFrameworkProductAssembly>
    <TargetFramework>$(NetCoreAppToolCurrent)</TargetFramework>
    <DefineConstants>READYTORUN;$(DefineConstants)</DefineConstants>
-    <Platforms>x64;x86;arm;arm64</Platforms>
+    <Platforms>x64;x86;arm;arm64;</Platforms>


Why is this ';' added here? Did something change in MSBuild that requires lists to be finished with a semicolon?

ivdiazsa · 2024-01-02T18:51:58Z

src/coreclr/tools/aot/ILCompiler.Reflection.ReadyToRun/GCInfoTypes.cs

@@ -154,6 +154,11 @@ internal GcInfoTypes(Machine machine)
                    STACK_BASE_REGISTER_ENCBASE = 2;
                    NUM_REGISTERS_ENCBASE = 3;
                    break;
+                case (Machine)0x5064: /* TODO: update with RiscV64 */


Same question as before. Do we have a tracking work item to change these hardcoded values to the respective enum values?

gbalykov · 2024-01-03T09:18:10Z

@davidwrighton @ivdiazsa I've created new issue for those todos #96438.

@davidwrighton It would be better to merge the change now and fix todos in separate PR. Can you please trigger rerun of CI? @ashaurtaev will get back from vacation only at the beginning of next week. So if CI passes after rerun, please merge this and we'll fix todos next week.

MichalStrehovsky · 2024-01-03T09:21:00Z

The easiest way to rerun the CI is to close and reopen the PR.

ivdiazsa · 2024-01-03T18:29:38Z

@gbalykov Thanks for filing the tracking issue for the TODO's!

gbalykov · 2024-01-04T05:28:24Z

Thank you!

ashaurtaev · 2024-01-09T09:28:12Z

Thank you!

Add crossgen2 for riscv64

46d78c3

ashaurtaev requested a review from MichalStrehovsky as a code owner November 23, 2023 22:41

ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 23, 2023

dotnet-issue-labeler bot added the area-crossgen2-coreclr label Nov 23, 2023

clamp03 assigned ashaurtaev Nov 23, 2023

clamp03 added the arch-riscv Related to the RISC-V architecture label Nov 23, 2023

clamp03 reviewed Nov 23, 2023

View reviewed changes

clamp03 suggested changes Nov 24, 2023

View reviewed changes

clamp03 reviewed Nov 24, 2023

View reviewed changes

clamp03 suggested changes Nov 27, 2023

View reviewed changes

tomeksowi suggested changes Nov 27, 2023

View reviewed changes

sirntar reviewed Nov 27, 2023

View reviewed changes

t-mustafin mentioned this pull request Dec 15, 2023

[question] crossgen2 generates misaligned relocations #96066

Open

Fix review comments

abeb033

build-analysis bot mentioned this pull request Dec 21, 2023

Tracking issue for CI build timeouts #76454

Closed

clamp03 reviewed Dec 21, 2023

View reviewed changes

tomeksowi approved these changes Dec 21, 2023

View reviewed changes

JongHeonChoi reviewed Dec 22, 2023

View reviewed changes

Fix review

98dbb02

gbalykov approved these changes Dec 22, 2023

View reviewed changes

build-analysis bot mentioned this pull request Dec 22, 2023

System.Tests.StringTests failing on windows Release NativeAOT_Libraries leg #96279

Closed

clamp03 approved these changes Dec 23, 2023

View reviewed changes

JongHeonChoi approved these changes Dec 25, 2023

View reviewed changes

clamp03 requested review from trylek and mangod9 December 26, 2023 23:49

t-mustafin approved these changes Dec 27, 2023

View reviewed changes

davidwrighton approved these changes Jan 2, 2024

View reviewed changes

ivdiazsa reviewed Jan 2, 2024

View reviewed changes

gbalykov mentioned this pull request Jan 3, 2024

[RISC-V] Update TODOs for RiscV64 Machine and Architecture in Crossgen2 #96438

Closed

MichalStrehovsky closed this Jan 3, 2024

MichalStrehovsky reopened this Jan 3, 2024

This was referenced Jan 3, 2024

Checkout failure: "Git fetch failed with exit code 128" dotnet/arcade#9009

Open

[mono][tvos] OOM in System.IO.Tests.MemoryStreamTests #92467

Closed

davidwrighton merged commit 5a2d151 into dotnet:main Jan 3, 2024
152 checks passed

ashaurtaev deleted the add_crossgen2_riscv64 branch January 9, 2024 09:28

dkurt mentioned this pull request Jan 15, 2024

[RISC-V] Optimizing assemblies for size failed #96978

Closed

gbalykov mentioned this pull request Jan 22, 2024

Enable riscv64 R2R in installer #96941

Merged

github-actions bot locked and limited conversation to collaborators Feb 9, 2024

		case RelocType.IMAGE_REL_BASED_RISCV64_PC:
		case RelocType.IMAGE_REL_BASED_RISCV64_JALR:

	code = 0x00000017;
	(code_t)dstRW = code \| (code_t)reg1 << 7 \| ((imm + 0x800) & 0xfffff000);
	dstRW += 4;
	#ifdef DEBUG
	code = emitInsCode(INS_auipc);
	assert(code == 0x00000017);
	code = emitInsCode(INS_addi);
	assert(code == 0x00000013);
	#endif
	ins = INS_addi;
	(code_t)dstRW = 0x00000013 \| ((code_t)reg1 << 7) \| ((code_t)reg1 << 15) \| ((doff & 0xfff) << 20);

	private static unsafe int GetRiscV64PC12(uint* pCode)
	private static unsafe int GetRiscV64PC(uint* pCode)

	private static unsafe void PutRiscV64PC12(uint* pCode, long imm32)
	private static unsafe void PutRiscV64PC(uint* pCode, long imm32)

		uint offset = symbol.RepresentsIndirectionCell ? 14u : 4u;
		uint encodedOffset = ((offset & 0x1e) << 7) \| ((offset & 0x7e0) << 20) \| ((offset & 0x800) >> 4) \| ((offset & 0x1000) << 19);

[RISC-V] Add crossgen2 for riscv64 #95188

[RISC-V] Add crossgen2 for riscv64 #95188

Conversation

ashaurtaev commented Nov 23, 2023

ashaurtaev commented Nov 23, 2023

clamp03 commented Nov 23, 2023 • edited Loading

clamp03 Nov 23, 2023 • 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

clamp03 Nov 24, 2023 • 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

clamp03 Nov 24, 2023 • 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

clamp03 Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

clamp03 Nov 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clamp03 commented Nov 24, 2023 • 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

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

gbalykov commented Dec 21, 2023 • edited Loading

ashaurtaev commented Dec 21, 2023

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

clamp03 Dec 22, 2023 • 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

clamp03 commented Nov 23, 2023 •

edited

Loading

clamp03 Nov 23, 2023 •

edited

Loading

clamp03 Nov 24, 2023 •

edited

Loading

clamp03 Nov 24, 2023 •

edited

Loading

clamp03 Nov 24, 2023 •

edited

Loading

clamp03 Nov 24, 2023 •

edited

Loading

clamp03 commented Nov 24, 2023 •

edited

Loading

gbalykov commented Dec 21, 2023 •

edited

Loading

clamp03 Dec 22, 2023 •

edited

Loading