[PATCH] D116305: [CSInfo][clang][ISEL][RISCV] Support for recovering optimized debug function parameters

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 added a comment.

This whole scheme seems like a pretty bad idea, is that really how the debug infrastructure works, scavenging generated code to figure out what arguments were passed rather than just remembering from the input? This all seems pretty fragile. I mean, if that's what we need to do to make it work then I guess we have to, but I really do not like this code...



================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:1910
+
+  // TODO: Special RISCV instructions that need to be described separately.
+  if (auto RegImm = isAddImmediate(MI, Reg)) {
----------------
Such as?....


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:1935
+
+Optional<RegImmPair> RISCVInstrInfo::isAddImmediate(const MachineInstr &MI,
+                                                    Register Reg) const {
----------------
Does this need to be a member function? It doesn't seem to access any state from what I can tell, only its inputs.

Also, the name is misleading. isAddImmediate suggests it's checking for RISCV::ADDI (which would be a pointless function), but it in fact does many more things than that.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:1943
+  case RISCV::ADDI: {
+    const MachineOperand &Dop = MI.getOperand(0);
+    const MachineOperand &Sop1 = MI.getOperand(1);
----------------
This naming convention isn't used anywhere else in the backend that I know of


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:1963
+      if (Sop1.getReg() == RISCV::X0 || Sop2.getImm() == (uint64_t)0)
+        return RegImmPair{RISCV::X0, (uint64_t)0};
+    break;
----------------
Does this case really matter for this? Why would we ever generate ANDI X0, 0 as the input to a function? Same goes for a whole bunch of these cases, that would be terrible codegen if we generate them.


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

https://reviews.llvm.org/D116305