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

[Digger via Phabricator via llvm-commits]

DiggerLin marked 11 inline comments as done.
DiggerLin added inline comments.


================
Comment at: llvm/include/llvm/Object/XCOFFObjectFile.h:399
 class TBVectorExt {
   friend class XCOFFTracebackTable;
 
----------------
jasonliu wrote:
> Let's remove this "friend" so that no one could call the constructor directly.
thanks


================
Comment at: llvm/lib/BinaryFormat/XCOFF.cpp:255
+      ParmsType += "vf";
+      break;
+    }
----------------
jasonliu wrote:
> default assert here?
there 4 cases for 2 bits, I do not think we need default here.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:923-924
   // Begin to parse optional fields.
   if (Cur) {
-    unsigned ParmNum = getNumberOfFixedParms() + getNumberOfFPParms();
-
-    // As long as there are no "fixed-point" or floating-point parameters, this
-    // field remains not present even when hasVectorInfo gives true and
-    // indicates the presence of vector parameters.
-    if (ParmNum > 0) {
-      uint32_t ParamsTypeValue = DE.getU32(Cur);
-      if (Cur)
-        ParmsType = hasVectorInfo()
-                        ? parseParmsTypeWithVecInfo(ParamsTypeValue, ParmNum)
-                        : parseParmsType(ParamsTypeValue, ParmNum);
-    }
+    if (FixedParmsNum + FloatingParmsNum > 0)
+      ParamsTypeValue = DE.getU32(Cur);
----------------
jasonliu wrote:
> 
thanks


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1959
+
+    CommentOS << "Parameter type = " << ParmsType.get();
     EmitComment();
----------------
jasonliu wrote:
> Only checking error in assert won't work in non-assertion build.
thanks


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:2006
+        // VR is saved on the stack.
+        VRData |= TracebackTable::IsVRSavedOnStackMask;
+        break;
----------------
jasonliu wrote:
> I think we are only supposed to set this bit when the special register VRSAVE is saved on stack.
Hubert point out that XL sets the bit (at least when there are other vector registers saved) , maybe we need double confirm with hubert @hubert.reinterpretcast 


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:2019
+
+    for (unsigned Reg = PPC::V0; Reg <= PPC::V31; ++Reg)
+      if (MRI.isPhysRegUsed(Reg)) {
----------------
jasonliu wrote:
> There is also a VSCR register, do we need to check that? Or are we inferring checking V0 to V31 would be enough to find out if any vmx instruction is used? If so, a comment would be desired. 
I do not think we need to check the VSCR. for "Special instructions
(mfvscr and mtvscr) are provided to move the VSCR from and to a vector register"
when operated on the vscr, there must be Vector register , we can check register PPC::V0 ~PPC::V31 .


================
Comment at: llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp:94
+  if (!HasVectorInfo && (Type >= VectorChar && Type <= VectorFloat))
+    HasVectorInfo = true;
+
----------------
jasonliu wrote:
> What if we don't have parameters that are vectors, but we use vectors inside of the function? I thought we should set HasVectorInfo in those situation as well. 
thanks for point out.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93659