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

[Nemanja Ivanovic via Phabricator via libcxx-commits]

nemanjai 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"
----------------
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")
```


================
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) \
----------------
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.


================
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"
----------------
Similarly here, we can simply set the vector registers to different values using `vspltisb`.


================
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
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138667