[RV64_DYNAREC] Added AVX.0F 10/11 opcode #1967

zohanzephyr · 2024-10-28T02:57:02Z

No description provided.

ksco

Requested changes. Also, would be great to have clang-formatted for all the newly added RV64 codes, thanks.

(Again, AVX infra for RV64 is not ready yet, things might change dramatically, would be great if you could wait for the infra to finish if you want to get involved. Meanwhile, SSE vector infra is very mature and welcomes contributions for new opcodes).

ksco · 2024-10-28T05:30:37Z

src/dynarec/rv64/dynarec_rv64_avx_0f.c

+            SSE_LOOP_MV_Q(x3);
+            if(vex.l) {
+                GETGY();
+                GETEY(x2, 0, 16);


Hmm, this seems not correct, how is the 16 calculated? Also, GETEY seems (very) expensive.

Hmm, this seems not correct, how is the 16 calculated? Also, GETEY seems (very) expensive.

Because the operation is on 256 bits of data

it should be 24. but like I said, the GETEY is too expensive and probably wrong, just reuse the result from GETEX.

Running you current GETEY after GETEX will endup running geted 2 times for the opcode, wich might result is reading more bytes from the opcode than actually exist. You should really looks athow it's done on the ARM64 side: geted is only called at the GETEX side, not on GETEY.
Also, with what program or game are you testing your changed?

Running you current GETEY after GETEX will endup running geted 2 times for the opcode, wich might result is reading more bytes from the opcode than actually exist. You should really looks athow it's done on the ARM64 side: geted is only called at the GETEX side, not on GETEY. Also, with what program or game are you testing your changed?

It is also possible to call it only on the GETGX side, the fixedaddress address is the same, but fixedaddress +=16 is correct, offset by 128 to get the value of ymm.

Running you current GETEY after GETEX will endup running geted 2 times for the opcode, wich might result is reading more bytes from the opcode than actually exist. You should really looks athow it's done on the ARM64 side: geted is only called at the GETEX side, not on GETEY. Also, with what program or game are you testing your changed?

A test demo I wrote myself

#include <stdio.h>

int main() {
float src[8] = {1.0f, 2.0f, 3.0f, 4.0f, 5.0f, 6.0f, 7.0f, 8.0f};
float dst[8] = {0.0f, 0.0f, 0.0f, 0.0f, 0.0f, 0.0f, 0.0f, 0.0f};

__asm__ volatile ( "vmovups (%1), %%ymm0\n" "vmovups %%ymm0, (%0)\n" : : "r" (dst), "r" (src) : "%ymm0" ); printf("Source data: %f %f %f %f %f %f %f %f\n", src[0], src[1], src[2], src[3], src[4], src[5], src[6], src[7]); printf("Destination data: %f %f %f %f %f %f %f %f\n", dst[0], dst[1], dst[2], dst[3], dst[4], dst[5], dst[6], dst[7]); return 0;

}
~

looks like a valid sample code (did you look at the box64 dump to see what is generated code for it?).
Feel free to add a few more opcode and add this the box64 test suite if it makes sense.

looks like a valid sample code (did you look at the box64 dump to see what is generated code for it?). Feel free to add a few more opcode and add this the box64 test suite if it makes sense.

Ok, geted should indeed be executed once, I've made the change

looks like a valid sample code (did you look at the box64 dump to see what is generated code for it?). Feel free to add a few more opcode and add this the box64 test suite if it makes sense.

So, the VMOVUPS didn't have SIB or Offset involved in this sample, that's why calling 2* times geted still worked. It would have crash and burn if an offset was involved in the opcode for example. Maybe try to use only 1 single array, instead of two?

(pro tips: use less -R dump.txt to see the dump in all it's colorfull glory)

ksco · 2024-10-28T05:30:46Z

src/dynarec/rv64/dynarec_rv64_avx_0f.c

+            SSE_LOOP_MV_Q2(x3);
+            if(vex.l) {
+                GETGY();
+                GETEY(x2, 0, 16);   


Hmm, this seems not correct, how is the 16 calculated? Also, GETEY seems (very) expensive.

ksco · 2024-10-28T05:31:17Z

src/dynarec/rv64/dynarec_rv64_avx_0f.c

+    #if STEP > 1
+    static const int8_t mask_shift8[] = { -7, -6, -5, -4, -3, -2, -1, 0 };
+    #endif


this is not used.

this is not used.

Yes, it's not being used at the moment

ksco · 2024-10-28T05:35:56Z

src/dynarec/rv64/dynarec_rv64_avx_0f.c

+    switch(opcode) {
+
+        case 0x10:
+            INST_NAME("VMOVUPS Gx,Ex");


s/Gx,Ex/Gx, Ex/

ksco · 2024-10-28T05:36:17Z

src/dynarec/rv64/dynarec_rv64_avx_0f.c

+            if(!vex.l) YMM0(gd);
+            break;
+        case 0x11:
+            INST_NAME("VMOVUPS Ex,Gx");


s/Ex,Gx/Ex, Gx/

ksco · 2024-10-28T05:39:19Z

src/dynarec/rv64/dynarec_rv64_helper.h

+        SMREAD();                                                                                \
+        ed = 16;                                                                                 \
+        addr = geted(dyn, addr, ninst, nextop, &wback, a, x3, &fixedaddress, rex, NULL, I12, D); \
+        fixedaddress+=16;                                                                        \


I don't think we can do fixedaddress += 16 here, the value will overflow.

I don't think we can do fixedaddress += 16 here, the value will overflow.

When fetching 256 bits of data, it needs to be offset by 128 bits

I don't think we can do fixedaddress += 16 here, the value will overflow.

It won't overflow, I did test it with the demo and the result is correct

please make sure you understand what the i12 is for.

ksco · 2024-10-28T10:57:16Z

src/dynarec/rv64/dynarec_rv64_helper.h

+    gback = xEmu;                               \
+    gdoffset = offsetof(x64emu_t, ymm[gd])
+
+#define GETEY(a, D, I12)                            \


I12 is useless.

ksco · 2024-10-28T11:00:09Z

src/dynarec/rv64/dynarec_rv64_avx_0f.c

+            INST_NAME("VMOVUPS Gx,Ex");
+            nextop = F8;
+            GETGX();
+            GETEX(x2, 0, 8);


8 is not correct, it should be 24.

ksco · 2024-10-29T06:01:22Z

this PR is still not correct, the extcache should be kept up to date.

ptitSeb · 2024-11-01T18:05:50Z

@zohanzephyr there are still some changes to be done on this PR.

zohanzephyr and others added 3 commits October 28, 2024 10:55

[RV64_DYNAREC] Added AVX.0F 10/11 opcode

a4d6154

Update CMakeLists.txt to include new file

d223ddf

Update dynarec_rv64_avx_0f.c

13f2122

ksco requested changes Oct 28, 2024

View reviewed changes

zohanzephyr added 2 commits October 28, 2024 15:22

Apply clang-format to newly added codes for consistent styling

f8a4824

Modify the GETGY

03f8a07

ksco requested changes Oct 28, 2024

View reviewed changes

parameter modification

68e91ae

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RV64_DYNAREC] Added AVX.0F 10/11 opcode #1967

[RV64_DYNAREC] Added AVX.0F 10/11 opcode #1967

zohanzephyr commented Oct 28, 2024

ksco left a comment •

edited

Loading

ksco Oct 28, 2024

zohanzephyr Oct 28, 2024

ksco Oct 28, 2024

ptitSeb Oct 28, 2024

zohanzephyr Oct 28, 2024

zohanzephyr Oct 28, 2024

ptitSeb Oct 28, 2024

zohanzephyr Oct 28, 2024

zohanzephyr Oct 28, 2024

ptitSeb Oct 28, 2024

ksco Oct 28, 2024

ksco Oct 28, 2024

zohanzephyr Oct 28, 2024

ksco Oct 28, 2024

ksco Oct 28, 2024

ksco Oct 28, 2024

zohanzephyr Oct 28, 2024

zohanzephyr Oct 28, 2024

ksco Oct 28, 2024

ksco Oct 28, 2024

ksco Oct 28, 2024

ksco commented Oct 29, 2024

ptitSeb commented Nov 1, 2024

[RV64_DYNAREC] Added AVX.0F 10/11 opcode #1967

Are you sure you want to change the base?

[RV64_DYNAREC] Added AVX.0F 10/11 opcode #1967

Conversation

zohanzephyr commented Oct 28, 2024

ksco left a comment • 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

ksco commented Oct 29, 2024

ptitSeb commented Nov 1, 2024

ksco left a comment •

edited

Loading