[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/BinaryFormat/XCOFF.cpp:142
+
+  if (Value != 0u || (ParsedFixedNum > FixedParmsNum) ||
+      (ParsedFloatingNum > FloatingParmsNum))
----------------
Something is wrong if `Value != 0u`, but I think it's normal when we have more parameters than the uint32 type could encode. And in those situation, as Hubert suggested, we could print `...` to represent any values that we could not decode. Please have a test case for this case as well.
Same for parseParmsTypeWithVecInfo function.


================
Comment at: llvm/lib/Object/XCOFFObjectFile.cpp:852
+
+TBVectorExt::TBVectorExt(StringRef TBvectorStrRef, Error &Err) {
   const uint8_t *Ptr = reinterpret_cast<const uint8_t *>(TBvectorStrRef.data());
----------------
This Error as an parameter in the constructor is too easy to get ignored. If we need to do this, we would do the whole set which includes making this constructor private, and have a Create function to return an error or the object, which is described here: https://llvm.org/docs/ProgrammersManual.html#fallible-constructors


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:2043
+
+    CommentOS << "Vector Parameter type = " << VecParmsType.get();
+    EmitComment();
----------------
This .get() won't work if you are compiling in a non-assert mode.


================
Comment at: llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp:70
+  unsigned Num = 0;
+  for (auto &Elt : ParamtersType)
+    if (Elt == FixedType)
----------------
Please fix all clang-tidy warnings.


================
Comment at: llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp:70
+  unsigned Num = 0;
+  for (auto &Elt : ParamtersType)
+    if (Elt == FixedType)
----------------
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()?


================
Comment at: llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp:107
+    case VectorChar:
+      VectExtParamInfo <<= XCOFF::TracebackTable::WidthOfParamType;
+      VectExtParamInfo |=
----------------
You could pull this out of the every case, and put it just before the switch statement. As you are doing it for every case.


================
Comment at: llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp:110
+          XCOFF::TracebackTable::ParmTypeIsVectorCharBit >> ShiftBits;
+      Bits += XCOFF::TracebackTable::WidthOfParamType;
+      break;
----------------
You could pull this out of the every case, and put it just after the switch statement. As you are doing it for every case.


================
Comment at: llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp:131
+    default:
+      break;
+    }
----------------
An assert here would be good.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93659