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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
