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

[Jason Liu via Phabricator via llvm-commits]

jasonliu added inline comments.


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


================
Comment at: llvm/lib/BinaryFormat/XCOFF.cpp:229-230
+        "in parseParmsTypeWithVecInfo.");
+  else
+    return ParmsType;
+}
----------------
minor style nit.


================
Comment at: llvm/lib/BinaryFormat/XCOFF.cpp:255
+      ParmsType += "vf";
+      break;
+    }
----------------
default assert 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);
----------------



================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:961
+      if (!TBVecExtOrErr) {
+        ErrorAsOutParameter EAO(&Err);
+        Err = TBVecExtOrErr.takeError();
----------------
I thought we only need to do this ErrorAsOutParameter once at the beginning of the function. Why do we need to do it 3 times at here and 981 and 992?


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


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:2006
+        // VR is saved on the stack.
+        VRData |= TracebackTable::IsVRSavedOnStackMask;
+        break;
----------------
I think we are only supposed to set this bit when the special register VRSAVE is saved on stack.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:2019
+
+    for (unsigned Reg = PPC::V0; Reg <= PPC::V31; ++Reg)
+      if (MRI.isPhysRegUsed(Reg)) {
----------------
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. 


================
Comment at: llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp:94
+  if (!HasVectorInfo && (Type >= VectorChar && Type <= VectorFloat))
+    HasVectorInfo = true;
+
----------------
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. 


================
Comment at: llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp:100
+uint32_t PPCFunctionInfo::getVecExtParmsType() const {
+  assert(HasVectorInfo && "There is not vector paramter in the function.");
+  uint32_t VectExtParamInfo = 0;
----------------



================
Comment at: llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp:191
+      break;
+    default:
+      assert(false && "Unrecognized type in ParamtersType");
----------------
Address the Lint comment. 


================
Comment at: llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp:70
+  unsigned Num = 0;
+  for (auto &Elt : ParamtersType)
+    if (Elt == FixedType)
----------------
DiggerLin wrote:
> jasonliu wrote:
> > jasonliu wrote:
> > > Please fix all clang-tidy warnings.
> > Instead of traversing this array for 3 times (getFixedParmsNum, getFloatingPointParmsNum, getVectorParmsNum), why not just still increment the corresponding data member in appendParameterType()?
> I think we only use here for getFloatingPointParmsNum, getVectorParmsNum once.
> I can increment corresponding data member , but it need three new data member here.
It's possible these functions got called later for other purposes and we don't know.
If we need new data members then we could just add it, I don't think the memory consumption of three unsigned types are too bad (comparing to three suboptimal traverse).


================
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
----------------
Is it possible to have tests for these fields?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93659