[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/CodeGen/MachineRegisterInfo.h:861-862
   /// function. This checks that no machine operands exist for the register or
-  /// any of its aliases. The register is also considered used when it is set
-  /// in the UsedPhysRegMask.
-  bool isPhysRegUsed(MCRegister PhysReg) const;
+  /// any of its aliases. if SkipTestUsedPhysRegMask is false, The register is
+  /// also considered used when it is set in the UsedPhysRegMask.
+  bool isPhysRegUsed(MCRegister PhysReg,
----------------
Please change accordingly for the `isPhysRegModified` above as well.


================
Comment at: llvm/include/llvm/CodeGen/MachineRegisterInfo.h:857
+  bool isPhysRegModified(MCRegister PhysReg, bool SkipNoReturnDef = false,
+                         bool SkipTestUsedPhysRegMask = false) const;
 
----------------
DiggerLin wrote:
> jasonliu wrote:
> > If this is necessary, could we add a test case for it? Right now, even setting the third parameter to always be false, I don't see any lit failing. 
> I do not think we need to new test case ,
> in the function MachineRegisterInfo::isPhysRegUsed , we change the 
> 
> if (UsedPhysRegMask.test(PhysReg)) 
>   return true;
> 
> ---> 
> 
> if (!SkipTestUsedPhysRegMask && UsedPhysRegMask.test(PhysReg))
>     return true;
> 
> and the the default value of SkipTestUsedPhysRegMask  is false,  it means , all the behaviors not change.
> 
> the behaviors only changes when SkipTestUsedPhysRegMask   =true;
> 
> and we have invoked
> MRI.isPhysRegModified(Reg, false, true) in several place in the function  PPCAIXAsmPrinter::emitTracebackTable. 
> I think we have test  SkipTestUsedPhysRegMask   =true too.
> 
Discussed offline, we need to add a test if the extra parameter is needed. 


================
Comment at: llvm/lib/BinaryFormat/XCOFF.cpp:146-147
+
+  if (Value != 0u || (ParsedFixedNum > FixedParmsNum) ||
+      (ParsedFloatingNum > FloatingParmsNum))
+    return createStringError(errc::invalid_argument,
----------------
nit


================
Comment at: llvm/lib/BinaryFormat/XCOFF.cpp:151
+                             "parameters in parseParmsType.");
+  else
+    return ParmsType;
----------------
Nit: Remove else.


================
Comment at: llvm/lib/BinaryFormat/XCOFF.cpp:269
+                             "in parseVectorParmsType.");
+  else
+    return ParmsType;
----------------
nit: Remove else


================
Comment at: llvm/lib/CodeGen/MachineRegisterInfo.cpp:587
+bool MachineRegisterInfo::isPhysRegUsed(MCRegister PhysReg,
+                                        bool SkipTestUsedPhysRegMask) const {
+  if (!SkipTestUsedPhysRegMask && UsedPhysRegMask.test(PhysReg))
----------------
I think this parameter name is too verbose. Suggest:
SkipTestUsedPhysRegMask -> SkipRegMaskTest


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:984
+
+  // As long as there are no "fixed-point" or floating-point parameters, this
+  // field remains not present even when hasVectorInfo gives true and
----------------
nit


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:998
+    }
+    ParmsType = ParmsTypeOrError.get();
   }
----------------
Is llvm-objdump capable of decoding the traceback table? Do we want to have a test case using llvm-objdump? Or are we thinking to put it on another patch?


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:2050
+  for (unsigned Reg = PPC::V0; Reg <= PPC::V31; ++Reg)
+    if (MRI.isPhysRegUsed(Reg, true)) {
+      // Has VMX instruction.
----------------
Could we add a comment saying that we would only treat instructions uses vector register as vector instructions?


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:2050
+  for (unsigned Reg = PPC::V0; Reg <= PPC::V31; ++Reg)
+    if (MRI.isPhysRegUsed(Reg, true)) {
+      // Has VMX instruction.
----------------
jasonliu wrote:
> Could we add a comment saying that we would only treat instructions uses vector register as vector instructions?
For the places that we use raw boolean value, could we have a comment beside to state what that value is for?
i.e.
if (MRI.isPhysRegUsed(Reg, /* SkipRegMaskTest */ true)) {



================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:2200-2205
+    for (unsigned Reg = PPC::V0; Reg <= PPC::V31; ++Reg)
+      if (!MRI.reg_nodbg_empty(Reg)) {
+        // Has VMX instruction.
+        VRData |= TracebackTable::HasVMXInstructionMask;
+        break;
+      }
----------------



================
Comment at: llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp:82
+  if (Type >= VectorChar && Type <= VectorFloat)
+    ++VectorParmsNum;
+}
----------------
Could we use a switch statement here instead instead of ifs? Also `report_fatal_error` if the Type is something we don't know. 



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93659