[PATCH] D139808: [RISCV] Add support for call returns to RISCVSExtWRemoval.

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.

Left a few comments based on an initial shallow review.

Is it possible to test the zext case, or is it too fiddly to construct a reasonable test case?



================
Comment at: llvm/lib/Target/RISCV/RISCVSExtWRemoval.cpp:370
+      if (CopySrcReg == RISCV::X10) {
+        // For a method return value, we check the ZExt/SExt flags in attribute.
+        // We assume the following code sequence for method call.
----------------
I think the note in your commit message "We use the PseudoCall to look up the IR function being called to find it's return attributes." is helpful to comprehension.

How about "For a method return value, we check the ZExt/SExt attributes, using the PseudoCALL instruction to retrieve the IR function."?


================
Comment at: llvm/lib/Target/RISCV/RISCVSExtWRemoval.cpp:376
+        const MachineBasicBlock *MBB = MI->getParent();
+        MachineBasicBlock::const_instr_iterator II =
+            MachineBasicBlock::const_instr_iterator(MI);
----------------
auto?


================
Comment at: llvm/lib/Target/RISCV/RISCVSExtWRemoval.cpp:386
+
+        const Function *CalleeFn =
+            dyn_cast_if_present<Function>(CallMI.getOperand(0).getGlobal());
----------------
Maybe auto? We're not overly consistent on this in LLVM and it's short anyway. But it would match the use of `auto` a few lines down.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139808