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

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro added inline comments.
Herald added a subscriber: pcwang-thead.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:1915-1916
+
+    // When SrcReg is $x0, treat loaded value as immediate only, $x0 is a zero
+    // register. Ex. $x10 = ADDI $x0, 10
+    if (SrcReg == RISCV::X0)
----------------
This comment sounds confusing -- please fix it.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:1935
+
+Optional<RegImmPair> RISCVInstrInfo::isAddImmediate(const MachineInstr &MI,
+                                                    Register Reg) const {
----------------
jrtc27 wrote:
> 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.
I guess a more appropriate name would be something like `isAddImmLike()`


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


================
Comment at: llvm/test/DebugInfo/MIR/RISCV/dbg-call-site-param.mir:485-516
+exposesReturnsTwice: false
+legalized:       false
+regBankSelected: false
+selected:        false
+failedISel:      false
+tracksRegLiveness: true
+hasWinCFI:       false
----------------
I believe we don't need all of these attrs.


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

https://reviews.llvm.org/D116305