[libcxx-commits] [PATCH] D138667: [libc++abi][LIT][AIX] Use Vector instructions available on Power7 in vec_reg_restore.pass.cpp

[Xing Xue via Phabricator via libcxx-commits]

xingxue marked 4 inline comments as done.
xingxue added inline comments.


================
Comment at: libcxxabi/test/vendor/ibm/vec_reg_restore.pass.cpp:45
   // Set VR63=100.
+  long long buff[2];
   asm volatile("li 30, 100\n\t"
----------------
nemanjai wrote:
> This test case can be significantly simplified by just materializing a value into the callee-saved registers you're looking to clobber. This implementation is potentially problematic because:
> - It is not immediately obvious what this is doing
> - I believe that the use of `"r"` asm constraint here for a memory location isn't what we want (I think `"Z"` or at least `"b"` would be needed here as the register cannot be `r0`).
> 
> I would recommend just something like this:
> ```
> // Set vs63 to 16 bytes each with value 9
> asm volatile("vspltisb 31, 9" : : : "v31");
> 
> // Set vs62 to 16 bytes each with value 12
> asm volatile("vspltisb 30, 12" : : : "v30")
> ```
Good idea, changed as suggested.  Thanks!


================
Comment at: libcxxabi/test/vendor/ibm/vec_reg_restore.pass.cpp:67-83
+#define getFirstValue(output, buff) \
+   asm volatile("li 30, 0\n\t" \
+                "stxvd2x 63, %[b], 30\n\t" \
+                "ld 30, 0(%[b])\n\t" \
+                "std 30, %[d]" \
+                : [d] "=rm"(output) \
+                : [b] "r"(buff) \
----------------
nemanjai wrote:
> I understand that this was here in the original test case, but I find it odd that we are comparing just the first doubleword of each vector. Although it is unlikely, we would miss any corruption in the second doubleword of each vector.
Changed to comparing all 16 bytes as suggested, thanks!


================
Comment at: libcxxabi/test/vendor/ibm/vec_reg_restore.pass.cpp:87
   // Set VR63=1.
   asm volatile("li 30, 1\n\t"
+               "std 30, 0(%[b])\n\t"
----------------
nemanjai wrote:
> Similarly here, we can simply set the vector registers to different values using `vspltisb`.
Changed as suggested, thanks!


================
Comment at: libcxxabi/test/vendor/ibm/vec_reg_restore.pass.cpp:113-114
     long long new_value2;
-    getFirstValue(new_value);
-    getSecondValue(new_value2);
+    getFirstValue(new_value, buff);
+    getSecondValue(new_value2, buff);
     // If the unwinder restores VR63 and VR62 correctly, they should contain
----------------
nemanjai wrote:
> Rather than doing this, what I would recommend is that we populate 2 new vectors with expected values and then compare them against `v31/v30` using `vcmpequb.`.
> You can use something like this for comparing the vectors:
> ```
> #define cmp63(vec, res) { \
>   vector unsigned char gbg; \
>   asm volatile("vcmpequb. %[gbg], 31, %[veca];" \
>                "mfocrf %[res], 2;" \
>                "rlwinm %[res], %[res], 25, 31, 31" \
>                : [res] "=r" (res), [gbg] "=v" (gbg) \
>                : [veca] "v" (vec) \
>                : "cr6" \
>               ); \
> }
> 
> #define cmp62(vec, res) { \
>   vector unsigned char gbg; \
>   asm volatile("vcmpequb. %[gbg], 30, %[veca];" \
>                "mfocrf %[res], 2;" \
>                "rlwinm %[res], %[res], 25, 31, 31" \
>                : [res] "=r" (res), [gbg] "=v" (gbg) \
>                : [veca] "v" (vec) \
>                : "cr6" \
>               ); \
> }
> 
> ```
> The results for these two will be placed in variable you pass as the second operand to the macro and the value should be `1` if the vectors are equal.
Good suggestion, changed. Thanks!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D138667/new/

https://reviews.llvm.org/D138667