[llvm] [RISCV] Implement RISCVInstrInfo::isAddImmediate (PR #72356)

Alex Bradbury via llvm-commits llvm-commits at lists.llvm.org
Wed Nov 15 22:26:09 PST 2023


================
@@ -2438,6 +2438,23 @@ MachineBasicBlock::iterator RISCVInstrInfo::insertOutlinedCall(
   return It;
 }
 
+std::optional<RegImmPair> RISCVInstrInfo::isAddImmediate(const MachineInstr &MI,
+                                                         Register Reg) const {
+  // TODO: Handle cases where Reg is a super- or sub-register of the
+  // destination register.
+  const MachineOperand &Op0 = MI.getOperand(0);
+  if (!Op0.isReg() || Reg != Op0.getReg())
+    return std::nullopt;
+
+  // Don't consider ADDIW as a candidate because the caller may not be aware
+  // of its sign extension behaviour.
+  if (MI.getOpcode() == RISCV::ADDI && MI.getOperand(1).isReg() &&
----------------
asb wrote:

We do. I was hoping to land this change and then follow up with testing and improvements for describeLoadedValue. I'm not really sure whether landing this patch as-is would be considered to regress current behaviour or not. isAddImmediate was always returning false before so we weren't generating the DIExpression for these cases before, and even without the Mips special casing the handling of LI doesn't _seem_ incorrect to my untrained eye (it's just that the reg+imm could be more efficiently represented as just imm in the case that reg==x0). I'm not a debuginfo expert so maybe there is a requirement that such expressions are simplified?

I think the LI case would be better handled in a target-independent way by having describeLoadedValue use getConstValDefinedInReg before then trying isAddImmediate. I'll take a look at how disruptive that change might be for other targets.

https://github.com/llvm/llvm-project/pull/72356


More information about the llvm-commits mailing list