[RISCV] Lower inline asm constraint A for RISC-V

[lewis-revill]

Summary

This allows arguments with the constraint A to be lowered to input nodes for RISC-V, which implies a memory address stored in a register.

This patch adds the minimal amount of code required to get operands with the right constraints to compile.

[lewis-revill]

Don't output an additional '0' when printing indirect memory operands.

[jrtc27]

I think we should introduce a new Constraint_A enum member for this case. Whilst "A" currently behaves the same for the RISC-V backend as "m", that won't necessarily be the case forever. "A" is required to always be a single GPR (and so can be used for the atomic instructions, or ones with reduced immediate encoding space), but "m" (at least according to GCC) is "any kind of address that the machine supports in general", which I assume for RISC-V means reg+simm12.

[lewis-revill]

Add Constraint_A to account for future modifications to Constraint_m handling. Constraint_A should only ever utilize a single base address without an offset.

[asb]

Added a couple of comments. Once addressed, should be fine. Thanks!

lib/Target/RISCV/RISCVAsmPrinter.cpp

118 ↗

(On Diff #204506)

Why make this change? It doesn't seem to match the GNU tools default, which I'd rather we stay in line with.

$ cat t.s 
lw a0, 0(a0)
$ ./riscv32-unknown-elf-as t.s
$ ./riscv32-unknown-elf-objdump -d a.out 

a.out:     file format elf32-littleriscv


Disassembly of section .text:

00000000 <.text>:
   0:	00052503          	lw	a0,0(a0)

lib/Target/RISCV/RISCVISelDAGToDAG.cpp

182

Doesn't need to be an extra case given the body is the same as the others.

[jrtc27]

lib/Target/RISCV/RISCVAsmPrinter.cpp

118 ↗

(On Diff #204506)

Because we can’t differentiate between A and m constraints here. A has no immediate and is suitable for all loads and stores, including atomics, so GCC will emit just (reg) matching the canonical form for atomics (GNU as also accepts an immediate that evaluates to zero and discards it). Whereas m can have an immediate, though we hard-coded it to 0 here. We accept lw x1, (x2) but we don’t accept (or at least certainly didn’t used to accept) lr.w x1, 0(x2), so we have to emit the form that works for everything, even if it’s not canonical for immediate loads and stores.

[lewis-revill]

lib/Target/RISCV/RISCVISelDAGToDAG.cpp

182

I added this to differentiate between the 'm' and 'A' because of the possibility of more complex 'm' constraints like @jrtc27 mentioned, IE including both an offset and a base.

[jrtc27]

lib/Target/RISCV/RISCVAsmPrinter.cpp

118 ↗

(On Diff #204506)

Ok, I was wrong, GCC will actually emit it as 0(reg), which only works because GNU as accepts lr.w rd, 0(rs1) despite not taking an immediate. We have two options:

1. Emit (rs1) (this patch) and rely on the existing lw rd, (rs1) aliases
2. Emit 0(rs1) (current behaviour) and add new lr.w rd, 0(rs1)/sc.w rd, rs2, 0(rs1)/amo rd, rs2, 0(rs1) aliases

[lenary]

lib/Target/RISCV/RISCVAsmPrinter.cpp

118 ↗

(On Diff #204506)

I've talked this over with @asb, and we're going to go with option 2. I'm working on a patch for that this morning, I will add it as a parent patch to this once it's ready.

[lewis-revill]

lib/Target/RISCV/RISCVAsmPrinter.cpp

118 ↗

(On Diff #204506)

Ok, will update this patch once this is done.

[lenary]

lib/Target/RISCV/RISCVAsmPrinter.cpp

118 ↗

(On Diff #204506)

Patch posted: D65205. I also added it as a parent patch.

[lewis-revill]

Rebased and updated to use '0(' prefix for memory constraints.

[asb]

Looks good to me - thanks!

[lewis-revill]

Rebased prior to commit.

[lewis-revill]

Committed (ill formed commit message so phabricator did not pick it up)

include/llvm/IR/InlineAsm.h

enum : uint32_t {
     // there's plenty of space in the encoding to support the union of all
     // constraint codes for all targets.
     Constraint_Unknown = 0,
     Constraint_es,
     Constraint_i,
     Constraint_m,
     Constraint_o,
     Constraint_v,
+    Constraint_A,
     Constraint_Q,
     Constraint_R,
     Constraint_S,
     Constraint_T,
     Constraint_Um,
     Constraint_Un,
     Constraint_Uq,
     Constraint_Us,

lib/Target/RISCV/RISCVISelDAGToDAG.cpp

bool RISCVDAGToDAGISel::SelectInlineAsmMemoryOperand(
     const SDValue &Op, unsigned ConstraintID, std::vector<SDValue> &OutOps) {
   switch (ConstraintID) {
   case InlineAsm::Constraint_i:
   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);
     return false;
+  case InlineAsm::Constraint_A:
+    OutOps.push_back(Op);
+    return false;
   default:
     break;
   }
 
   return true;
 }
 
 bool RISCVDAGToDAGISel::SelectAddrFI(SDValue Addr, SDValue &Base) {

lib/Target/RISCV/RISCVISelLowering.h

public:
   unsigned ComputeNumSignBitsForTargetNode(SDValue Op,
                                            const APInt &DemandedElts,
                                            const SelectionDAG &DAG,
                                            unsigned Depth) const override;
 
   // This method returns the name of a target specific DAG node.
   const char *getTargetNodeName(unsigned Opcode) const override;
 
+  // Get the type of an inline asm constraint.
+  ConstraintType getConstraintType(StringRef Constraint) const override;
+
+  unsigned getInlineAsmMemConstraint(StringRef ConstraintCode) const override;
+
   std::pair<unsigned, const TargetRegisterClass *>
   getRegForInlineAsmConstraint(const TargetRegisterInfo *TRI,
                                StringRef Constraint, MVT VT) const override;
 
   void LowerAsmOperandForConstraint(SDValue Op, std::string &Constraint,
                                     std::vector<SDValue> &Ops,
                                     SelectionDAG &DAG) const override;
 

lib/Target/RISCV/RISCVISelLowering.cpp

const char *RISCVTargetLowering::getTargetNodeName(unsigned Opcode) const {
   case RISCVISD::FMV_X_ANYEXTW_RV64:
     return "RISCVISD::FMV_X_ANYEXTW_RV64";
   case RISCVISD::READ_CYCLE_WIDE:
     return "RISCVISD::READ_CYCLE_WIDE";
   }
   return nullptr;
 }
 
+RISCVTargetLowering::ConstraintType
+RISCVTargetLowering::getConstraintType(StringRef Constraint) const {
+  // Currently only support length 1 constraints.
+  if (Constraint.size() == 1) {
+    switch (Constraint[0]) {
+    case 'A':
+      return C_Memory;
+    default:
+      break;
+    }
+  }
+
+  return TargetLowering::getConstraintType(Constraint);
+}
+
 std::pair<unsigned, const TargetRegisterClass *>
 RISCVTargetLowering::getRegForInlineAsmConstraint(const TargetRegisterInfo *TRI,
                                                   StringRef Constraint,
                                                   MVT VT) const {
   // First, see if this is a constraint that directly corresponds to a
   // RISCV register class.
   if (Constraint.size() == 1) {
     switch (Constraint[0]) {
     case 'r':
       return std::make_pair(0U, &RISCV::GPRRegClass);
     default:
       break;
     }
   }
 
   return TargetLowering::getRegForInlineAsmConstraint(TRI, Constraint, VT);
 }
 
+unsigned
+RISCVTargetLowering::getInlineAsmMemConstraint(StringRef ConstraintCode) const {
+  // Currently only support length 1 constraints.
+  if (ConstraintCode.size() == 1) {
+    switch (ConstraintCode[0]) {
+    case 'A':
+      return InlineAsm::Constraint_A;
+    default:
+      break;
+    }
+  }
+
+  return TargetLowering::getInlineAsmMemConstraint(ConstraintCode);
+}
+
 void RISCVTargetLowering::LowerAsmOperandForConstraint(
     SDValue Op, std::string &Constraint, std::vector<SDValue> &Ops,
     SelectionDAG &DAG) const {
   // Currently only support length 1 constraints.
   if (Constraint.length() == 1) {
     switch (Constraint[0]) {
     case 'I':
       // Validate & create a 12-bit signed immediate operand.

test/CodeGen/RISCV/inline-asm.ll

 ; RV64I-NEXT:    csrwi mstatus, 0
 ; RV64I-NEXT:    #NO_APP
 ; RV64I-NEXT:    ret
   tail call void asm sideeffect "csrwi mstatus, $0", "K"(i32 31)
   tail call void asm sideeffect "csrwi mstatus, $0", "K"(i32 0)
   ret void
 }
 
+define void @constraint_A(i8* %a) nounwind {
+; RV32I-LABEL: constraint_A:
+; RV32I:       # %bb.0:
+; RV32I-NEXT:    #APP
+; RV32I-NEXT:    sb s0, 0(a0)
+; RV32I-NEXT:    #NO_APP
+; RV32I-NEXT:    #APP
+; RV32I-NEXT:    lb s1, 0(a0)
+; RV32I-NEXT:    #NO_APP
+; RV32I-NEXT:    ret
+;
+; RV64I-LABEL: constraint_A:
+; RV64I:       # %bb.0:
+; RV64I-NEXT:    #APP
+; RV64I-NEXT:    sb s0, 0(a0)
+; RV64I-NEXT:    #NO_APP
+; RV64I-NEXT:    #APP
+; RV64I-NEXT:    lb s1, 0(a0)
+; RV64I-NEXT:    #NO_APP
+; RV64I-NEXT:    ret
+  tail call void asm sideeffect "sb s0, $0", "*A"(i8* %a)
+  tail call void asm sideeffect "lb s1, $0", "*A"(i8* %a)
+  ret void
+}
+
 define i32 @modifier_z_zero(i32 %a) nounwind {
 ; RV32I-LABEL: modifier_z_zero:
 ; RV32I:       # %bb.0:
 ; RV32I-NEXT:    #APP
 ; RV32I-NEXT:    add a0, a0, zero
 ; RV32I-NEXT:    #NO_APP
 ; RV32I-NEXT:    ret
   %1 = tail call i32 asm "add $0, $1, ${2:z}", "=r,r,r"(i32 %a, i32 0)