[PATCH] D93659: [AIX][XCOFF] emit vector info of traceback table.

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1882
   for (unsigned Reg = PPC::F14; Reg <= PPC::F31; ++Reg) {
-    if (MRI.isPhysRegModified(Reg)) {
+    if (isPhysRegModified(Reg)) {
       FPRSaved = PPC::F31 - Reg + 1;
----------------
I have concerns about writing our own version of this function.
I think it's important to understand why the MRI version doesn't work. Is it a bug or work as intended? If writing our own version is really necessary, I would avoid naming them the same as it's going to cause confusion for people on when to use which version. 


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:2028
+
+        // In IBM XL compiler, it sets the bit when VR is saved on the stack.
+        VRData |= TracebackTable::IsVRSavedOnStackMask;
----------------



================
Comment at: llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable-vectorinfo.ll:56
+; COMMON-NEXT:  .byte   102                             # Function Name
+; COMMON-NEXT:  .byte   0x00                            # NumOfVRsSaved = 0, -IsVRSavedOnStack, -HasVarArgs
+; COMMON-NEXT:  .byte   0x07                            # NumOfVectorParams = 3, +HasVMXInstruction
----------------
DiggerLin wrote:
> jasonliu wrote:
> > Is it possible to have tests for these fields?
> I can not add a test case for it.  There is  error as " error in backend: variadic arguments for vector types are unimplemented for AIX" 
Sure. I saw you add a test case to test the error for HasVarArgs, which is good. 
But what about NumOfVRsSaved = 0, -IsVRSavedOnStack? I don't think that get affected by the error. 


================
Comment at: llvm/test/CodeGen/PowerPC/aix-emit-tracebacktable-vectorinfo_hasvarg.ll:5
+
+;; #include <altivec.h>
+;; vector float f(vector float vf, int x,...) {
----------------
I don't think we want to use include in the lit test. You could write the declaration yourself. And you probably don't even need to call vec_abs if you just want to check if HasVarArgs bit is set?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93659