[PATCH] D99812: [PowerPC] [GlobalISel] Implementation of formal arguments lowering in the IRTranslator for the PPC backend

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCCallLowering.cpp:46-58
+/**
+ * @brief Lower incoming arguments into generic MIR, this method is responsible
+ *  for splitting aggregate arguments into multiple single value types as well
+ *  as setting argument flags for each argument. This method depends on a
+ *  calling convention selector to select the correct calling convention based
+ *  on the F.getCallingConv(). Finally, FormalArgHandler takes care of the reg
+ *  assignments.
----------------
There's no reason to duplicate the documentation effort from the function this is overriding. This will just add bitrotting comments


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCCallLowering.cpp:69-71
+  // initialize instruction writer
+  if (!MBB.empty())
+    MIRBuilder.setInstr(*MBB.begin());
----------------
This is probably duplicated in every target, but I don't think it is necessary (or it shouldn't be, the caller should have set the insert point appropriately)


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCCallLowering.cpp:73
+
+  // loop over each arg, set flags and split to single value types
+  SmallVector<ArgInfo, 8> InArgs;
----------------
Capitalize


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCCallLowering.cpp:99
+/**
+ * @brief generates COPYs of values to registers and G_TRUNCs them whenever
+ *  the bit widths mismatch. Formal arguments lowering depends on this method
----------------
Same as above


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCCallLowering.cpp:111-138
+  switch (VA.getLocInfo()) {
+  default: {
+    // If we are copying the value from a physical register with the
+    // size larger than the size of the value itself - build the copy
+    // of the phys reg first and then build the truncation of that copy.
+    // The example of that would be copying from xmm0 to s32, for which
+    // case ValVT == LocVT == MVT::f32. If LocSize and ValSize are not equal
----------------
Can you just rely on the default implementation?

I've been working on a patch for a while to try to clean all of this up. Essentially what's happening is GlobalISel is using the CCAssignFns in a way that's subtly incompatible with how the DAG uses it. All of the code in this function ends up hacking around this, and I would at least prefer to keep this consolidated in one place to fix.


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCCallLowering.cpp:143
+/**
+ * @brief Generate a load instruction to load value from the given address.
+ * @param ValVReg
----------------
Same as above


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCCallLowering.cpp:168-169
+  // and then truncate it
+  if (VA.getLocInfo() == CCValAssign::SExt or
+      VA.getLocInfo() == CCValAssign::ZExt) {
+    Size = 4;
----------------
You shouldn't rely on the extension hint to know if the result size is larger, it's not necessarily true


================
Comment at: llvm/lib/Target/PowerPC/GISel/PPCCallLowering.cpp:174
+  }
+  // otherwise, simply load the address into the destination register
+  else {
----------------
Weird comment between the control flow blocks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D99812