[RISCV] Lower inline asm m with offset to register+imm.

[mikhail.ramalho]

Summary

As part of D145584, we noticed that llvm was generating suboptimal code
for constraint m when the operand can be be lowered to reg+imm form: it
was being selected as a single register rather than register+imm. This
caused an unnecessary 'addi' to be gen for each m constraint.

This patch changes llvm to select register+imm. This might generate code
that cannot be assembled, but matches gcc's behavior.

[asb]

I don't feel comfortable approving this because I played a hand in diagnosing the issue and helping Mikhail with the patch. I do think updating our handling of the 'm' constraint to match GCC's is the right thing to do, and this patch achieves it in the simplest way I'm aware of. The logic around asm operand constraints isn't always easy to follow, so it's possible I'm missing alternative ways of achieving the same goal.

[kito-cheng]

LGTM, and m in GCC has some special treatment for vector types, it will forbid offset, but it's kind of because GCC's implementation, so I am not asking to match this behavior.

That's remind me that we should have document those into riscv-c-api-doc or some where...I has a draft but never finished, I guess I should find some time to create a PR to start the discussion.

#include <riscv_vector.h>
int x[100];

int foo(){
    vint32m1_t *a = (vint32m1_t *)&(x[1]); // little bit hack but just used for easier demo the difference
    int y,z;
    asm ("lw %0, %1": "=r"(y) : "m"(*a));
    asm ("lw %0, %1": "=r"(z) : "m"(x[1]));
    return y+z;
}

[craig.topper]

This comment has been deleted.

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp

185

Is "Expanded" here supposed to be "Expected"?

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

2102

The body of an assert is not executed in release builds. SelectAddRegImm will not be called.

[kito-cheng]

FYI https://github.com/riscv-non-isa/riscv-c-api-doc/pull/33

[mikhail.ramalho]

• Fixed error message
• Fixed function not being called in release mode

[craig.topper]

LGTM

llvm/lib/Target/RISCV/RISCVAsmPrinter.cpp

 
 bool RISCVAsmPrinter::PrintAsmMemoryOperand(const MachineInstr *MI,
                                             unsigned OpNo,
                                             const char *ExtraCode,
                                             raw_ostream &OS) {
   if (ExtraCode)
     return AsmPrinter::PrintAsmMemoryOperand(MI, OpNo, ExtraCode, OS);
 
-  const MachineOperand &MO = MI->getOperand(OpNo);
-  // For now, we only support register memory operands in registers and
-  // assume there is no addend
-  if (!MO.isReg())
+  const MachineOperand &AddrReg = MI->getOperand(OpNo);
+  assert(MI->getNumOperands() > OpNo + 1 && "Expanded additional operand");
+  const MachineOperand &DispImm = MI->getOperand(OpNo + 1);
+  // All memory operands should have a register and an immediate operand (see
+  // RISCVDAGToDAGISel::SelectInlineAsmMemoryOperand).
+  if (!AddrReg.isReg())
+    return true;
+  if (!DispImm.isImm())
     return true;
 
-  OS << "0(" << RISCVInstPrinter::getRegisterName(MO.getReg()) << ")";
+  OS << DispImm.getImm() << "("
+     << RISCVInstPrinter::getRegisterName(AddrReg.getReg()) << ")";
   return false;
 }
 
 bool RISCVAsmPrinter::runOnMachineFunction(MachineFunction &MF) {
   STI = &MF.getSubtarget<RISCVSubtarget>();
 
   SetupMachineFunction(MF);
   emitFunctionBody();

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp

#undef CASE_VMXOR_VMANDN_VMOR_OPCODES
   }
 
   // Select the default instruction.
   SelectCode(Node);
 }
 
 bool RISCVDAGToDAGISel::SelectInlineAsmMemoryOperand(
     const SDValue &Op, unsigned ConstraintID, std::vector<SDValue> &OutOps) {
+  // Always produce a register and immediate operand, as expected by
+  // RISCVAsmPrinter::PrintAsmMemoryOperand.
   switch (ConstraintID) {
-  case InlineAsm::Constraint_m:
-    // We just support simple memory operands that have a single address
-    // operand and need no special handling.
-    OutOps.push_back(Op);
+  case InlineAsm::Constraint_m: {
+    SDValue Op0, Op1;
+    assert(SelectAddrRegImm(Op, Op0, Op1) &&
+           "SelectAddrRegImm should always succeed");
+    OutOps.push_back(Op0);
+    OutOps.push_back(Op1);
     return false;
+  }
   case InlineAsm::Constraint_A:
     OutOps.push_back(Op);
+    OutOps.push_back(
+        CurDAG->getTargetConstant(0, SDLoc(Op), Subtarget->getXLenVT()));
     return false;
   default:
     break;
   }
 
   return true;
 }
 

llvm/test/CodeGen/RISCV/inline-asm.ll

 ; RV64I-NEXT:    ret
   %1 = tail call i32 asm "lw $0, $1", "=r,*m"(ptr elementtype(i32) %a)
   ret i32 %1
 }
 
 define i32 @constraint_m_with_offset(ptr %a) nounwind {
 ; RV32I-LABEL: constraint_m_with_offset:
 ; RV32I:       # %bb.0:
-; RV32I-NEXT:    addi a0, a0, 4
 ; RV32I-NEXT:    #APP
-; RV32I-NEXT:    lw a0, 0(a0)
+; RV32I-NEXT:    lw a0, 4(a0)
 ; RV32I-NEXT:    #NO_APP
 ; RV32I-NEXT:    ret
 ;
 ; RV64I-LABEL: constraint_m_with_offset:
 ; RV64I:       # %bb.0:
-; RV64I-NEXT:    addi a0, a0, 4
 ; RV64I-NEXT:    #APP
-; RV64I-NEXT:    lw a0, 0(a0)
+; RV64I-NEXT:    lw a0, 4(a0)
 ; RV64I-NEXT:    #NO_APP
 ; RV64I-NEXT:    ret
   %1 = getelementptr i32, ptr %a, i32 1
   %2 = tail call i32 asm "lw $0, $1", "=r,*m"(ptr elementtype(i32) %1)
   ret i32 %2
 }
 
 define void @constraint_I() nounwind {