[RISC-V] Add DynamicHelpers to riscv64 stubs #94735

ashaurtaev · 2023-11-14T21:36:04Z

Added implementation of dynamic helpers to stubs for riscv64

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

t-mustafin · 2023-11-14T22:18:24Z

src/coreclr/vm/riscv64/stubs.cpp

+                    // add  t5, a0, t4
+                    *(DWORD*)p = 0x01d50f33; p += 4;
+                    // ld  t5, 0(t5)
+                    *(DWORD*)p = 0x000f3f03; 


Should here also be p += 4?

t-mustafin · 2023-11-14T22:34:17Z

src/coreclr/vm/riscv64/stubs.cpp

+
+    BEGIN_DYNAMIC_HELPER_EMIT(32);
+
+    *(DWORD*)p = 0x00000297;// auipc t0, 0


stubs.cpp for riscv64 has ITypeInstr, STypeInstr and RTypeInstr functions to encode instructions. I suppose usage this functions make code more readable and prevent complicated debugging.

t-mustafin · 2023-11-14T23:02:06Z

src/coreclr/vm/riscv64/stubs.cpp

+    p += 4;
+    *(DWORD*)p = 0x00028067;// jr t0
+    p += 4;
+


Seems padding nop should also be here.

t-mustafin · 2023-11-14T23:09:31Z

src/coreclr/vm/riscv64/stubs.cpp

+                *(DWORD*)p = 0x000eae83 | ((dataOffset & 0xfff) << 20);
+                p += 4;
+                // add a0, a0, t4
+                *(DWORD*)p = 0x01d50533; 


Seems p += 4 missed here.

clamp03 · 2023-11-14T23:54:38Z

src/coreclr/vm/riscv64/stubs.cpp

+
+            // CALL HELPER:
+            if(pBLECall != NULL)
+                *(DWORD*)pBLECall |= ((UINT32)(p - pBLECall) << 8);


I think this sets offset to pBLECall (blt t4, t5, CALL HELPER). Please check the below how offset is splitted into instruction.

// B-immediate encodes a signed offset in multiples of 2 bytes // 12 | 11 1 | 0 // inst[31]/sign | inst[7] | inst[30:25] | inst[11:8] | 0

+ Please add an assertion for offset.

I see why you shift only once. It is because max value of p - pBLECall under 5 bits (0-4). However, I think shift value should be 7 not 8. And please add assertions (0th is 0 and max value is under 5 bits(0-4)) for value of p - pBLECall.

clamp03 · 2023-11-15T00:01:24Z

src/coreclr/vm/riscv64/stubs.cpp

+        int indirectionsDataSize = 0;
+        if (pLookup->testForNull || pLookup->sizeOffset != CORINFO_NO_SIZE_CHECK)
+        {
+            codeSize += 4; //mv t2, 0


In the below, you added mv t2, a0 instruction. Could you update this comment?

clamp03 · 2023-11-15T00:11:42Z

src/coreclr/vm/riscv64/stubs.cpp

+        else
+        {
+            // beq  a0, x0, CALL HELPER:
+            *(DWORD*)p = 0x00050063;


I think it is wrong. beq a0, x0, 8 (offset to CALL HELPER) is 0x00050463.
Please use ITypeInstr, STypeInstr and RTypeInstr functions if possible.

clamp03 · 2023-11-15T00:15:46Z

src/coreclr/vm/riscv64/stubs.cpp

+
+        if (pLookup->testForNull)
+        {
+            codeSize += 12; // ori-beq-jalr


Please update comment to beq-ret-ori like actual code generation.

clamp03 · 2023-11-15T00:45:27Z

src/coreclr/vm/riscv64/stubs.cpp

+                if (pLookup->sizeOffset > 2047)
+                {
+                    // auipc  t4, 0
+                    *(DWORD*)p = 0x00000297; p += 4;


Isn't it better to use the previous t4 which is set when indirectionsDataSize is over zero? You can remove many auipc instructions, simplify complex dataOffset calculation and handle indirectionsDataSize as boolean variable. (However, You need to rename t4 which is used for lui.) If it is impossible, please remove previous auipc t4, 0.

I agree. This will remove both auipc instructions in for loop, and dataOffset will be relative not to current instruction, but to the beginning of whole code block. Also, it would be good to add assert that dataOffset relative to beginning of code block is <= 2047.

Comment with more details about this logic in #94766 (comment).

gbalykov · 2023-11-15T07:36:00Z

src/coreclr/vm/riscv64/stubs.cpp

+                if (pLookup->sizeOffset > 2047)
+                {
+                    // auipc  t4, 0
+                    *(DWORD*)p = 0x00000297; p += 4;


I agree. This will remove both auipc instructions in for loop, and dataOffset will be relative not to current instruction, but to the beginning of whole code block. Also, it would be good to add assert that dataOffset relative to beginning of code block is <= 2047.

gbalykov · 2023-11-15T07:38:45Z

src/coreclr/vm/riscv64/stubs.cpp

+            {
+                _ASSERTE(dataOffset < 2047);
+                // auipc  t4, 0
+                *(DWORD*)p = 0x00000297;


@shushanhf it seems that for loongarch you also either need to add pcaddi for if(pLookup->offsets[i] > 2047) case (because dataOffset is relative to current instruction), or change dataOffset to be relative to the beginning of whole code block.

The LA64 had done this.

runtime/src/coreclr/vm/loongarch64/stubs.cpp

Lines 1883 to 1891 in 18cac43

if(pLookup->offsets[i] > 2047)

{

_ASSERTE(dataOffset < 2047);

// ld.wu $t4,$r21,0

*(DWORD*)p = 0x2a8002b0 | (dataOffset<<10);

p += 4;

// ldx.d $a0,$a0,$t4

*(DWORD*)p = 0x380c4084;

p += 4;

I know what you said,

runtime/src/coreclr/vm/loongarch64/stubs.cpp

Lines 1854 to 1855 in 18cac43

// pcaddi $r21,0

*(DWORD*)p = 0x18000015; p += 4;

Here the r21 was overwritten.

@gbalykov What should I do ? I push a new PR to fix this or update it within this PR ?

I think you can do it in separate PR specific to loongarch

Thanks very much !

tomeksowi · 2023-11-15T12:27:30Z

src/coreclr/vm/riscv64/stubs.cpp

+    *(DWORD*)p = 0x00000297;// auipc t0, 0
+    p += 4;
+    *(DWORD*)p = 0x0102b503;// ld a0, 16(t0)
+    p += 4;
+    *(DWORD*)p = 0x0182b283;// ld t0, 24(t0)
+    p += 4;
+    *(DWORD*)p = 0x00028067;// jr t0
+    p += 4;
+
+    // label:
+    // arg
+    *(TADDR*)p = arg;
+    p += 8;
+    // target
+    *(PCODE*)p = target;
+    p += 8;


Looks like exactly the same sequence as EmitHelperWithArg, please deduplicate.

tomeksowi · 2023-11-15T12:37:45Z

src/coreclr/vm/riscv64/stubs.cpp

+            if (codeSize & 0x7)
+                codeSize += 4;


Suggested change

if (codeSize & 0x7)

codeSize += 4;

codeSize = ALIGN_UP(codeSize, 8);

tomeksowi · 2023-11-15T12:49:40Z

src/coreclr/vm/riscv64/stubs.cpp

+                *(DWORD*)p = 0x000e8e93 | ((slotOffset & 0xfff) << 20); p += 4;
+                dataOffset -= 8;
+                // blt  t4, t5, CALL HELPER
+                pBLECall = p;       // Offset filled later


Nit: shouldn't this var be named pBLTCall to match the instruction?

tomeksowi · 2023-11-15T13:39:20Z

src/coreclr/vm/riscv64/stubs.cpp

+    BEGIN_DYNAMIC_HELPER_EMIT(32);
+
+    *(DWORD*)p = 0x00000297;// auipc t0, 0
+    p += 4;
+    *(DWORD*)p = 0x0102b503;// ld a0, 16(t0)
+    p += 4;
+    *(DWORD*)p = 0x0182b283;// ld t0, 24(t0)
+    p += 4;
+    *(DWORD*)p = 0x00028067;// jr t0
+    p += 4;
+
+    // label:
+    // arg
+    *(TADDR*)p = arg;
+    p += 8;
+    // target
+    *(PCODE*)p = target;
+    p += 8;
+
+    END_DYNAMIC_HELPER_EMIT();


BTW, rather than manually calculate proper sizing and offsets each time, it could be useful to employ the compiler to do it for you, e.g. something like:

struct Asm { DWORD instructions[4] = { UTypeInstr(AUIPC, t0, 0), ITypeInstr(LOAD, LD, a0, t0, offsetof(Asm, _arg)), ... }; PCODE _arg = arg; PCODE _target = target; }; static_assert(offsetof(Asm, _arg) % sizeof(PCODE) == 0, "naturally align data constants"); BEGIN_DYNAMIC_HELPER_EMIT(sizeof(Asm)); *(Asm*)p = Asm{}; END_DYNAMIC_HELPER_EMIT();

It also has the marginal benefit of more optimal code generated for the emitter.

I left the logic of the functions by analogy with other architectures.

tomeksowi · 2023-11-15T14:35:45Z

src/coreclr/vm/riscv64/stubs.cpp

+                indirectionsDataSize += (pLookup->sizeOffset > 2047 ? 4 : 0);
+            }
+
+            codeSize += (pLookup->offsets[i] > 2047 ? 16 : 4); // if( > 2047) (8 bytes) else 4 bytes for instructions.


Comment says 8 bytes, the code says 16.

tomeksowi · 2023-11-15T14:41:37Z

src/coreclr/vm/riscv64/stubs.cpp

+            // ori  a0,t2,0
+            *(DWORD*)p = 0x0003e513;


Nit: I think the proper instruction for moving a register on RISC-V is addi.

It's more than just a nit — on an OoO CPU using the proper addi rd,rs,0 instruction is virtually guaranteed to result in a register rename with no execution resources needed, while it would be likely many or most cores would not do the same for the ori version (or xori ...,0 or andi ...,-1 or sll ...,0 etc).

brucehoult · 2023-11-16T00:45:29Z

There are no PC-relative loads or stores in RISC-V at present. (Qualcomm has a PC-relative load, but not store, in their controversial Zics proposal)

…

On Thu, Nov 16, 2023 at 4:02 AM Tomasz Sowiński ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/coreclr/vm/riscv64/stubs.cpp <#94735 (comment)>: > + *(DWORD*)p = 0x00000297;// auipc t0, 0 + p += 4; + *(DWORD*)p = 0x0102b503;// ld a0, 16(t0) + p += 4; I think this could be optimized to ld a0, 8(pc); the offset changed 16->8 because if you remove the auipc, the nop below is also unnecessary. — Reply to this email directly, view it on GitHub <#94735 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAGPYYA6OSLXZZNUMG23YDLYETKOZAVCNFSM6AAAAAA7LPRTV2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTOMZSGIZTQNBRHA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

shushanhf · 2023-11-16T01:28:40Z

@gbalykov
I have rewrite the dataOffset for LoongArch64, welcome to review #94766
By the way, maybe it's useful for RISCV64.

Thanks

tomeksowi · 2023-11-16T07:00:08Z

There are no PC-relative loads or stores in RISC-V at present. (Qualcomm has a PC-relative load, but not store, in their controversial Zics proposal)

Yeah, that's why I deleted the comment as soon as I realized that:) Thanks.

gbalykov · 2023-11-18T09:33:48Z

src/coreclr/vm/riscv64/stubs.cpp

    p += 4;
-    *(DWORD*)p = 0x00050513 | ((offset & 0xfff)<<20);// addi a0,a0,offset
+    *(DWORD*)p = ITypeInstr(INSTR_ADDI, FUNCT3_ADDI, REG_A0, REG_A0, ((offset & 0xfff)<<20));// addi a0, a0, offset


<<20 is not needed here

gbalykov · 2023-11-18T09:35:40Z

src/coreclr/vm/riscv64/stubs.cpp

-                    *(DWORD*)p = 0x01d50f33; p += 4;
-                    // ld  t5, 0(t5)
-                    *(DWORD*)p = 0x000f3f03; 
+                    *(DWORD*)p = ITypeInstr(INSTR_LW, FUNCT3_LW, REG_T4, REG_T4, (dataOffset << 20));// lw  t4, dataOffset(t4)


<<20 is not needed here

gbalykov · 2023-11-18T09:36:13Z

src/coreclr/vm/riscv64/stubs.cpp

-                    // ld  t5, #(pLookup->sizeOffset)(a0)
-                    *(DWORD*)p = 0x00053f03 | ((UINT32)pLookup->sizeOffset << 20); p += 4;
-                    dataOffset -= 4; // subtract 4 as we have moved PC by 4
+                    *(DWORD*)p = ITypeInstr(INSTR_LD, FUNCT3_LD, REG_T5, REG_A0, ((UINT32)pLookup->sizeOffset << 20));// ld  t5, #(pLookup->sizeOffset)(a0)


<<20 is not needed here

gbalykov · 2023-11-18T09:36:35Z

src/coreclr/vm/riscv64/stubs.cpp

-                dataOffset -= 8;
+                *(DWORD*)p = UTypeInstr(INSTR_LUI, REG_T4, ((((UINT32)slotOffset & 0xfffff000) >> 12) << 12));
+                p += 4;
+                *(DWORD*)p = ITypeInstr(INSTR_ADDI, FUNCT3_ADDI, REG_T4, REG_T4, ((slotOffset & 0xfff) << 20));// addi  t4, REG_T4, (slotOffset&0xfff)


<<20 is not needed here

gbalykov · 2023-11-18T09:37:25Z

src/coreclr/vm/riscv64/stubs.cpp

-                // addi  t4, t4, (slotOffset&0xfff)
-                *(DWORD*)p = 0x000e8e93 | ((slotOffset & 0xfff) << 20); p += 4;
-                dataOffset -= 8;
+                *(DWORD*)p = UTypeInstr(INSTR_LUI, REG_T4, ((((UINT32)slotOffset & 0xfffff000) >> 12) << 12));


<<12 is not needed here

gbalykov · 2023-11-18T09:37:41Z

src/coreclr/vm/riscv64/stubs.cpp

            }

            if(pLookup->offsets[i] > 2047)
            {
                _ASSERTE(dataOffset < 2047);
-                // auipc  t4, 0
-                *(DWORD*)p = 0x00000297;
+                *(DWORD*)p = ITypeInstr(INSTR_LW, FUNCT3_LW, REG_T4, REG_T4, ((dataOffset & 0xfff) << 20));// lw  t4, dataOffset(t4)


<<20 is not needed here

gbalykov · 2023-11-18T09:39:43Z

src/coreclr/vm/riscv64/stubs.cpp

@@ -1823,19 +1872,18 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator,
                indirectionsDataSize += (pLookup->sizeOffset > 2047 ? 4 : 0);
            }

-            codeSize += (pLookup->offsets[i] > 2047 ? 16 : 4); // if( > 2047) (8 bytes) else 4 bytes for instructions.
+            codeSize += (pLookup->offsets[i] > 2047 ? 16 : 4); // if( > 2047) (16 bytes) else 4 bytes for instructions.


in if above size is wrong, it seems that it should be codeSize += (pLookup->sizeOffset > 2047 ? 24 : 16);

it seems this one is also wrong:
codeSize += (pLookup->offsets[i] > 2047 ? 12 : 4); // if( > 2047) (12 bytes) else 4 bytes for instructions.

gbalykov · 2023-11-18T09:42:45Z

src/coreclr/vm/riscv64/stubs.cpp

            }
            else
            {
                // offset must be 8 byte aligned
                _ASSERTE((pLookup->offsets[i] & 0x7) == 0);
-                // ld  a0, #(pLookup->offsets[i])(a0)
-                *(DWORD*)p = 0x00053503 | ((UINT32)pLookup->offsets[i] << 20);
+                *(DWORD*)p = ITypeInstr(INSTR_LD, FUNCT3_LD, REG_A0, REG_A0, ((UINT32)pLookup->offsets[i] << 20));// ld  a0, #(pLookup->offsets[i])(a0)


<<20 is not needed here

clamp03 · 2023-11-19T23:06:26Z

src/coreclr/vm/riscv64/stubs.cpp

@@ -1507,14 +1515,78 @@ void StubLinkerCPU::EmitCallManagedMethod(MethodDesc *pMD, BOOL fTailCall)
 //
 // Allocation of dynamic helpers
 //
+enum Riscv64_registers
+{
+    REG_ZERO,


We usually use REG_R0.

Done, now this form of register writing is used const IntReg RegR0

clamp03 · 2023-11-19T23:30:49Z

src/coreclr/vm/riscv64/stubs.cpp

+    FUNCT3_LD = 0x3,
+    FUNCT3_LW = 0x2,
+    FUNCT3_JALR = 0,
+};


Could you follow the existing style in stubs.cpp?
REG is declared like const IntReg RegSp = IntReg(2); in vm/riscv64/cgencpu.h
instructions and funct3 are handled in Emit* functions. I think you need to update *TypeInstr to Emit* functions in this PR.

You can add registers, emit functions and BTypeInstr.
Thank you!

Created BTypeInstr to work with the instructions of this type. But the logic of Emit* is unclear since it uses a call to ITypeInstr or others functions. Therefore, I left the current logic of functions.

Okay. Emit32 cannot handle instructions well for your cases.

clamp03 · 2023-11-19T23:34:01Z

@ashaurtaev @gbalykov Even if it doesn't make crossgen2 work fully, I think you made simple tests on your own and checked them on your local. Could you share how you tested this PR?

gbalykov · 2023-11-20T09:13:43Z

@clamp03 there're no tests for this one separately, it can't be tested without crossgen2 (as far as I know). And crossgen2 itself is not yet functional. This one is just reviewed closely, and for the next PR we'll try to bring some tests.

clamp03 · 2023-11-21T02:14:43Z

src/coreclr/vm/riscv64/stubs.cpp

+                p += 4;
+                // blt  t4, t5, CALL HELPER
+                pBLTCall = p;       // Offset filled later
+                *(DWORD*)p = BTypeInstr(0x63, 0x4, RegT4, RegT5, 0);


I think it is better to remove this and update the below emittion like this.

if(pBLTCall != NULL) *(DWORD*)pBLTCall = BTypeInstr(0x63, 0x4, RegT4, RegT5, (UINT32)(p - pBLTCall)));

Removed this line and updated the instructions below.

clamp03 · 2023-11-21T04:10:18Z

src/coreclr/vm/riscv64/stubs.cpp

+    int immLo4 = (imm13 >> 1) & 0xf;
+    int immHi6 = (imm13 >> 5) & 0x3f;
+    int immHi1 = (imm13 >> 12) & 0x1;
+    return opcode | (immLo4 << 7) | (funct3 << 12) | (rs1 << 15) | (rs2 << 20) | (immHi6 << 25) | (immLo1 << 7) | (immHi1 << 31);


Update to immLo4 << 8.

Fixed it. Thank you!

clamp03 · 2023-11-21T05:36:08Z

src/coreclr/vm/riscv64/stubs.cpp

+        else
+        {
+            // beq  a0, x0, CALL HELPER:
+            *(DWORD*)p = BTypeInstr(0x63, 0, RegA0, RegR0, 0);


I think imm value should not be 0. It is 8? Please check again.

Fixed it. Thank you!

src/coreclr/vm/riscv64/stubs.cpp

tomeksowi · 2023-11-21T08:47:43Z

src/coreclr/vm/riscv64/stubs.cpp

+
+        if (pLookup->testForNull)
+        {
+            codeSize += 12; // beq-ret-ori


Suggested change

codeSize += 12; // beq-ret-ori

codeSize += 12; // beq-ret-addi

Fixed it. Thank you!

clamp03 · 2023-11-21T14:20:23Z

src/coreclr/vm/riscv64/stubs.cpp

+                *(DWORD*)p = ITypeInstr(0x13, 0, RegT4, RegT4, slotOffset & 0xfff);// addi  t4, t4, (slotOffset&0xfff)
+                p += 4;
+                // blt  t4, t5, CALL HELPER
+                pBLTCall = p;       // Offset filled later


p += 4; should not be removed. Please add p += 4;

gbalykov · 2023-11-21T16:21:45Z

src/coreclr/vm/riscv64/stubs.cpp

@@ -1611,13 +1570,15 @@ void DynamicHelpers::EmitHelperWithArg(BYTE*& p, size_t rxOffset, LoaderAllocato
 {
    STANDARD_VM_CONTRACT;

-    *(DWORD*)p = UTypeInstr(INSTR_AUIPC, REG_T0, 0);// auipc t0, 0
+    const IntReg RegR0 = 0, RegT0 = 5, RegA0 = 10, RegA1 = 11;


RegA1 is not used here

Removed RegA1

gbalykov · 2023-11-21T16:28:29Z

src/coreclr/vm/riscv64/stubs.cpp

@@ -1872,15 +1848,15 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator,
                indirectionsDataSize += (pLookup->sizeOffset > 2047 ? 4 : 0);


codeSize += (pLookup->sizeOffset > 2047 ? 24 : 16); above

gbalykov · 2023-11-21T16:29:13Z

src/coreclr/vm/riscv64/stubs.cpp

@@ -1872,15 +1848,15 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator,
                indirectionsDataSize += (pLookup->sizeOffset > 2047 ? 4 : 0);
            }

-            codeSize += (pLookup->offsets[i] > 2047 ? 16 : 4); // if( > 2047) (16 bytes) else 4 bytes for instructions.
+            codeSize += (pLookup->offsets[i] > 2047 ? 24 : 16); // if( > 2047) (24 bytes) else 16 bytes for instructions.


codeSize += (pLookup->offsets[i] > 2047 ? 12 : 4); // if( > 2047) (12 bytes) else 4 bytes for instructions.

I got it, thank you!

clamp03 · 2023-11-21T22:43:58Z

src/coreclr/vm/riscv64/stubs.cpp

+            codeSize += 12; // beq-ret-addi
+
+            //padding for 8-byte align (required by EmitHelperWithArg)
+            codeSize += ALIGN_UP(codeSize, 8);


Update to codeSize = ALIGN_UP(codeSize, 8);

src/coreclr/vm/riscv64/stubs.cpp

jkotas

Thanks

Add DynamicHelpers to riscv64 stubs

fe5c19f

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

dotnet-issue-labeler bot added the area-VM-coreclr label Nov 14, 2023

clamp03 assigned ashaurtaev Nov 14, 2023

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

t-mustafin reviewed Nov 14, 2023

View reviewed changes

clamp03 reviewed Nov 14, 2023

View reviewed changes

build-analysis bot mentioned this pull request Nov 15, 2023

Test_EventSource_EtwManifestGeneration* tests failing in CI #48798

Closed

clamp03 suggested changes Nov 15, 2023

View reviewed changes

gbalykov reviewed Nov 15, 2023

View reviewed changes

shushanhf mentioned this pull request Nov 15, 2023

[LoongArch64] Fix the overwritten of reg21 when pLookup->sizeOffset >2047 #94766

Merged

tomeksowi reviewed Nov 15, 2023

View reviewed changes

Code review feedback

cb2197f

ashaurtaev changed the title ~~Add DynamicHelpers to riscv64 stubs~~ [RISC-V] Add DynamicHelpers to riscv64 stubs Nov 17, 2023

build-analysis bot mentioned this pull request Nov 17, 2023

DisabledRuntimeMarshalling.PInvokeAssemblyMarshallingEnabled.DelegatesFromExternalAssembly.StructWithDefaultNonBlittableFields() failing #94931

Closed

gbalykov reviewed Nov 18, 2023

View reviewed changes

clamp03 reviewed Nov 19, 2023

View reviewed changes

clamp03 reviewed Nov 21, 2023

View reviewed changes

tomeksowi suggested changes Nov 21, 2023

View reviewed changes

ashaurtaev force-pushed the FixDynamicHelpers branch from c8dc3b3 to 7db6a3e Compare November 21, 2023 13:47

clamp03 suggested changes Nov 21, 2023

View reviewed changes

ashaurtaev force-pushed the FixDynamicHelpers branch from 7db6a3e to d114c47 Compare November 21, 2023 15:58

gbalykov reviewed Nov 21, 2023

View reviewed changes

Fixing code review remarks

8ada2fd

ashaurtaev force-pushed the FixDynamicHelpers branch from d114c47 to 8ada2fd Compare November 21, 2023 16:49

gbalykov approved these changes Nov 21, 2023

View reviewed changes

clamp03 reviewed Nov 21, 2023

View reviewed changes

Fix codeSize

d46ccd1

clamp03 approved these changes Nov 22, 2023

View reviewed changes

clamp03 requested a review from jkotas November 22, 2023 10:22

jkotas reviewed Nov 22, 2023

View reviewed changes

src/coreclr/vm/riscv64/stubs.cpp Outdated Show resolved Hide resolved

src/coreclr/vm/riscv64/stubs.cpp Outdated Show resolved Hide resolved

src/coreclr/vm/riscv64/stubs.cpp Outdated Show resolved Hide resolved

Apply suggestions from code review

f4967d0

jkotas approved these changes Nov 22, 2023

View reviewed changes

jkotas merged commit 2096625 into dotnet:main Nov 22, 2023
6 of 16 checks passed

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

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

Open

2 tasks

github-actions bot locked and limited conversation to collaborators Dec 23, 2023


		BEGIN_DYNAMIC_HELPER_EMIT(32);

		(DWORD)p = 0x00000297;// auipc t0, 0

	if(pLookup->offsets[i] > 2047)
	{
	_ASSERTE(dataOffset < 2047);
	// ld.wu $t4,$r21,0
	(DWORD)p = 0x2a8002b0 \| (dataOffset<<10);
	p += 4;
	// ldx.d $a0,$a0,$t4
	(DWORD)p = 0x380c4084;
	p += 4;

	if (codeSize & 0x7)
	codeSize += 4;
	codeSize = ALIGN_UP(codeSize, 8);

	codeSize += 12; // beq-ret-ori
	codeSize += 12; // beq-ret-addi

		@@ -1872,15 +1848,15 @@ PCODE DynamicHelpers::CreateDictionaryLookupHelper(LoaderAllocator * pAllocator,
		indirectionsDataSize += (pLookup->sizeOffset > 2047 ? 4 : 0);

[RISC-V] Add DynamicHelpers to riscv64 stubs #94735

[RISC-V] Add DynamicHelpers to riscv64 stubs #94735

Conversation

ashaurtaev commented Nov 14, 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 Nov 15, 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 15, 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

gbalykov Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shushanhf Nov 15, 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

tomeksowi Nov 15, 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

tomeksowi Nov 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brucehoult commented Nov 16, 2023 via email

shushanhf commented Nov 16, 2023

tomeksowi commented Nov 16, 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

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

clamp03 commented Nov 19, 2023

gbalykov commented Nov 20, 2023

clamp03 Nov 21, 2023 • edited Loading

Choose a reason for hiding this comment

clamp03 Nov 15, 2023 •

edited

Loading

clamp03 Nov 15, 2023 •

edited

Loading

gbalykov Nov 15, 2023 •

edited

Loading

shushanhf Nov 15, 2023 •

edited

Loading

tomeksowi Nov 15, 2023 •

edited

Loading

tomeksowi Nov 15, 2023 •

edited

Loading

clamp03 Nov 21, 2023 •

edited

Loading

clamp03 Nov 21, 2023 •

edited

Loading