[RISCV 12/n] Codegen support for memory operations

[asb]

Summary

As a reminder, the philosophy here is to avoid copying large portions of codes from other backends, and only add new code paths when they are both needed and can be tested. As such, even though there's nothing preventing this patch from supporting compilation with PIC, it's something that should come later. I feel this is a logical "next testable unit" after the initial ALU codegen.

[asb]

The patch has been updated with a new wide-mem.ll test case, several minor formatting issues have been fixed, and the mem.ll test has been updated to reflect upstream changes in instruction schedules.

[majnemer]

lib/Target/RISCV/RISCVISelLowering.cpp

90

Shouldn't this be report_fatal_error ?

[asb]

Fix inappropriate use of llvm_unreachable (thanks @majnemer!).

[asb]

Instructions definitions and patterns have been split, as discussed in http://lists.llvm.org/pipermail/llvm-dev/2017-August/116635.html

[mgrang]

lib/Target/RISCV/RISCVMCInstLower.cpp

53 ↗

(On Diff #112448)

Not sure about the indentation of this line. Can you run clang-format?

77 ↗

(On Diff #112448)

Same here.

[asb]

Fix indentation (thanks @mgrang!).

[asb]

lib/Target/RISCV/RISCVMCInstLower.cpp

53 ↗

(On Diff #112448)

This line was produced by clang-format.

77 ↗

(On Diff #112448)

This line was also produced by clang-format, however I notice indentation of this whole function was was broken during some refactoring. Fixed.

[kparzysz]

lib/Target/RISCV/RISCVISelLowering.h

32 ↗

(On Diff #114145)

Please make this a reference, since Subtarget cannot be nullptr.

lib/Target/RISCV/RISCVInstrInfo.td

318

Line just a bit too long.

333

This line is too long.

[asb]

Update based on review comments (thanks!). This version of the patch also adds (and tests) support for sextload/zextload/anyextload of an i1 value.

[glasnak]

lib/Target/RISCV/RISCV.h

21 ↗

(On Diff #115578)

this include is not really needed when you have "class MCInst;" below.
Same for including MachineOperand.h, wouldn't "class MachineOperand;" be sufficient?

lib/Target/RISCV/RISCVISelLowering.cpp

73

I'm honestly confused. Why is LowerOperation CamelCase, but lowerGlobalAddress is camelCase? I've seen this inconsistency of function naming elsewhere too, and all I can think of is that LLVM is changing to camelCase function code style just very very slowly. Is this correct?

Anyway, I'll leave the decision up to you, but these two functions have almost identical signature and a different style, that doesn't seem right.

lib/Target/RISCV/RISCVMCInstLower.cpp

25 ↗

(On Diff #115578)

This file now seems to be working perfectly fine with just these includes:
#include "RISCV.h"
#include "MCTargetDesc/RISCVMCExpr.h"
#include "llvm/CodeGen/AsmPrinter.h"
#include "llvm/MC/MCAsmInfo.h"
#include "llvm/MC/MCContext.h"
please remove the others.

[asb]

lib/Target/RISCV/RISCVISelLowering.cpp

73

Yes, LLVM is very slowly moving to camelCase for function names. LowerOperation is called from target-independent code, so we're stuck with the old naming style for now.

[asb]

Clean up #includes.

[reames]

test/CodeGen/RISCV/alu.ll

164 ↗

(On Diff #116946)

These tests have nothing to do with memory access. You should separate a patch which implements this support.

test/CodeGen/RISCV/mem.ll

141

It looks like there's non-trivial support required specifically for globals. This should probably be separated into it's own patch.

You might also be able to start with a simpler (orthogonal) test. Consider:
define i32* @test() {

return i32* @G

}

[asb]

Updated to split constant materialization to a preceding patch (D39101) and global addresses to a succeeding patch (to be posted shortly).

[reames]

LGTM

Thanks for splitting. Tracking the pieces is now much easier.

lib/Target/RISCV/RISCVInstrInfo.cpp

39

This looks like it can just be an assert.

[asb]

Thanks for helping to unblock this Philip. I was holding off on committing as Chandler was hoping to corral another backend maintainer into giving a second opinion. Unfortunately it looks like that person didn't have time, so I'm going ahead and committing.

lib/Target/RISCV/RISCVISelLowering.cpp

RISCVTargetLowering::RISCVTargetLowering(const TargetMachine &TM,
   // Set up the register classes.
   addRegisterClass(XLenVT, &RISCV::GPRRegClass);
 
   // Compute derived properties from the register classes.
   computeRegisterProperties(STI.getRegisterInfo());
 
   setStackPointerRegisterToSaveRestore(RISCV::X2);
 
+  for (auto N : {ISD::EXTLOAD, ISD::SEXTLOAD, ISD::ZEXTLOAD})
+    setLoadExtAction(N, XLenVT, MVT::i1, Promote);
+
   // TODO: add all necessary setOperationAction calls.
 
   setBooleanContents(ZeroOrOneBooleanContent);
 
   // Function alignments (log2).
   setMinFunctionAlignment(3);
   setPrefFunctionAlignment(3);
 }
 
 SDValue RISCVTargetLowering::LowerOperation(SDValue Op,
                                             SelectionDAG &DAG) const {
   switch (Op.getOpcode()) {
   default:
     report_fatal_error("unimplemented operand");
   }
 }
 
 // Calling Convention Implementation.
 #include "RISCVGenCallingConv.inc"
 
 // Transform physical registers into virtual registers.
 SDValue RISCVTargetLowering::LowerFormalArguments(
     SDValue Chain, CallingConv::ID CallConv, bool IsVarArg,
     const SmallVectorImpl<ISD::InputArg> &Ins, const SDLoc &DL,
     SelectionDAG &DAG, SmallVectorImpl<SDValue> &InVals) const {
 
   switch (CallConv) {
   default:
     report_fatal_error("Unsupported calling convention");
   case CallingConv::C:
     break;
   }
 
   MachineFunction &MF = DAG.getMachineFunction();
   MachineRegisterInfo &RegInfo = MF.getRegInfo();
   MVT XLenVT = Subtarget.getXLenVT();
 
   if (IsVarArg)
     report_fatal_error("VarArg not supported");
 
   // Assign locations to all of the incoming arguments.
   SmallVector<CCValAssign, 16> ArgLocs;
   CCState CCInfo(CallConv, IsVarArg, MF, ArgLocs, *DAG.getContext());
   CCInfo.AnalyzeFormalArguments(Ins, CC_RISCV32);

lib/Target/RISCV/RISCVInstrInfo.h

 #include "RISCVGenInstrInfo.inc"
 
 namespace llvm {
 
 class RISCVInstrInfo : public RISCVGenInstrInfo {
 
 public:
   RISCVInstrInfo();
+
+  void copyPhysReg(MachineBasicBlock &MBB, MachineBasicBlock::iterator Position,
+                   const DebugLoc &DL, unsigned DestinationRegister,
+                   unsigned SourceRegister, bool KillSource) const override;
 };
 }
 
 #endif

lib/Target/RISCV/RISCVInstrInfo.cpp

 #include "llvm/Support/TargetRegistry.h"
 
 #define GET_INSTRINFO_CTOR_DTOR
 #include "RISCVGenInstrInfo.inc"
 
 using namespace llvm;
 
 RISCVInstrInfo::RISCVInstrInfo() : RISCVGenInstrInfo() {}
+
+void RISCVInstrInfo::copyPhysReg(MachineBasicBlock &MBB,
+                                 MachineBasicBlock::iterator Position,
+                                 const DebugLoc &DL,
+                                 unsigned DestinationRegister,
+                                 unsigned SourceRegister,
+                                 bool KillSource) const {
+  if (!RISCV::GPRRegClass.contains(DestinationRegister, SourceRegister)) {
+    llvm_unreachable("Impossible reg-to-reg copy");
+  }
+
+  BuildMI(MBB, Position, DL, get(RISCV::ADDI), DestinationRegister)
+      .addReg(SourceRegister, getKillRegState(KillSource))
+      .addImm(0);
+}

lib/Target/RISCV/RISCVInstrInfo.td

 def : PatGprGpr<setult, SLTU>;
 def : PatGprSimm12<setult, SLTIU>;
 
 /// Branches and jumps
 
 let isBarrier = 1, isReturn = 1, isTerminator = 1 in
 def PseudoRET : Pseudo<(outs), (ins), [(RetFlag)]>,
                 PseudoInstExpansion<(JALR X0, X1, 0)>;
+
+/// Loads
+
+multiclass LdPat<PatFrag LoadOp, RVInst Inst> {
+  def : Pat<(LoadOp GPR:$rs1), (Inst GPR:$rs1, 0)>;
+  def : Pat<(LoadOp (add GPR:$rs1, simm12:$imm12)),
+            (Inst GPR:$rs1, simm12:$imm12)>;
+}
+
+defm : LdPat<sextloadi8, LB>;
+defm : LdPat<extloadi8, LB>;
+defm : LdPat<sextloadi16, LH>;
+defm : LdPat<extloadi16, LH>;
+defm : LdPat<load, LW>;
+defm : LdPat<zextloadi8, LBU>;
+defm : LdPat<zextloadi16, LHU>;
+
+/// Stores
+
+multiclass StPat<PatFrag StoreOp, RVInst Inst> {
+  def : Pat<(StoreOp GPR:$rs2, GPR:$rs1), (Inst GPR:$rs2, GPR:$rs1, 0)>;
+  def : Pat<(StoreOp GPR:$rs2, (add GPR:$rs1, simm12:$imm12)),
+            (Inst GPR:$rs2, GPR:$rs1, simm12:$imm12)>;
+}
+
+defm : StPat<truncstorei8, SB>;
+defm : StPat<truncstorei16, SH>;
+defm : StPat<store, SW>;

test/CodeGen/RISCV/mem.ll

+; RUN: llc -mtriple=riscv32 -verify-machineinstrs < %s \
+; RUN:   | FileCheck %s -check-prefix=RV32I
+
+; Check indexed and unindexed, sext, zext and anyext loads
+
+define i32 @lb(i8 *%a) nounwind {
+; RV32I-LABEL: lb:
+; RV32I: lb a1, 0(a0)
+; RV32I: lb a0, 1(a0)
+  %1 = getelementptr i8, i8* %a, i32 1
+  %2 = load i8, i8* %1
+  %3 = sext i8 %2 to i32
+  ; the unused load will produce an anyext for selection
+  %4 = load volatile i8, i8* %a
+  ret i32 %3
+}
+
+define i32 @lh(i16 *%a) nounwind {
+; RV32I-LABEL: lh:
+; RV32I: lh a1, 0(a0)
+; RV32I: lh a0, 4(a0)
+  %1 = getelementptr i16, i16* %a, i32 2
+  %2 = load i16, i16* %1
+  %3 = sext i16 %2 to i32
+  ; the unused load will produce an anyext for selection
+  %4 = load volatile i16, i16* %a
+  ret i32 %3
+}
+
+define i32 @lw(i32 *%a) nounwind {
+; RV32I-LABEL: lw:
+; RV32I: lw a1, 0(a0)
+; RV32I: lw a0, 12(a0)
+  %1 = getelementptr i32, i32* %a, i32 3
+  %2 = load i32, i32* %1
+  %3 = load volatile i32, i32* %a
+  ret i32 %2
+}
+
+define i32 @lbu(i8 *%a) nounwind {
+; RV32I-LABEL: lbu:
+; RV32I: lbu a1, 0(a0)
+; RV32I: lbu a0, 4(a0)
+  %1 = getelementptr i8, i8* %a, i32 4
+  %2 = load i8, i8* %1
+  %3 = zext i8 %2 to i32
+  %4 = load volatile i8, i8* %a
+  %5 = zext i8 %4 to i32
+  %6 = add i32 %3, %5
+  ret i32 %6
+}
+
+define i32 @lhu(i16 *%a) nounwind {
+; RV32I-LABEL: lhu:
+; RV32I: lhu a1, 0(a0)
+; RV32I: lhu a0, 10(a0)
+  %1 = getelementptr i16, i16* %a, i32 5
+  %2 = load i16, i16* %1
+  %3 = zext i16 %2 to i32
+  %4 = load volatile i16, i16* %a
+  %5 = zext i16 %4 to i32
+  %6 = add i32 %3, %5
+  ret i32 %6
+}
+
+; Check indexed and unindexed stores
+
+define void @sb(i8 *%a, i8 %b) nounwind {
+; RV32I-LABEL: sb:
+; RV32I: sb a1, 6(a0)
+; RV32I: sb a1, 0(a0)
+  store i8 %b, i8* %a
+  %1 = getelementptr i8, i8* %a, i32 6
+  store i8 %b, i8* %1
+  ret void
+}
+
+define void @sh(i16 *%a, i16 %b) nounwind {
+; RV32I-LABEL: sh:
+; RV32I: sh a1, 14(a0)
+; RV32I: sh a1, 0(a0)
+  store i16 %b, i16* %a
+  %1 = getelementptr i16, i16* %a, i32 7
+  store i16 %b, i16* %1
+  ret void
+}
+
+define void @sw(i32 *%a, i32 %b) nounwind {
+; RV32I-LABEL: sw:
+; RV32I: sw a1, 32(a0)
+; RV32I: sw a1, 0(a0)
+  store i32 %b, i32* %a
+  %1 = getelementptr i32, i32* %a, i32 8
+  store i32 %b, i32* %1
+  ret void
+}
+
+; Check load and store to an i1 location
+define i32 @load_sext_zext_anyext_i1(i1 *%a) nounwind {
+; RV32I-LABEL: load_sext_zext_anyext_i1:
+; RV32I: lb a1, 0(a0)
+; RV32I: lbu a1, 1(a0)
+; RV32I: lbu a0, 2(a0)
+; RV32I: sub a0, a0, a1
+; RV32I: jalr zero, ra, 0
+  ; sextload i1
+  %1 = getelementptr i1, i1* %a, i32 1
+  %2 = load i1, i1* %1
+  %3 = sext i1 %2 to i32
+  ; zextload i1
+  %4 = getelementptr i1, i1* %a, i32 2
+  %5 = load i1, i1* %4
+  %6 = zext i1 %5 to i32
+  %7 = add i32 %3, %6
+  ; extload i1 (anyext). Produced as the load is unused.
+  %8 = load volatile i1, i1* %a
+  ret i32 %7
+}
+
+define i16 @load_sext_zext_anyext_i1_i16(i1 *%a) nounwind {
+; RV32I-LABEL: load_sext_zext_anyext_i1_i16:
+; RV32I: lb a1, 0(a0)
+; RV32I: lbu a1, 1(a0)
+; RV32I: lbu a0, 2(a0)
+; RV32I: sub a0, a0, a1
+; RV32I: jalr zero, ra, 0
+  ; sextload i1
+  %1 = getelementptr i1, i1* %a, i32 1
+  %2 = load i1, i1* %1
+  %3 = sext i1 %2 to i16
+  ; zextload i1
+  %4 = getelementptr i1, i1* %a, i32 2
+  %5 = load i1, i1* %4
+  %6 = zext i1 %5 to i16
+  %7 = add i16 %3, %6
+  ; extload i1 (anyext). Produced as the load is unused.
+  %8 = load volatile i1, i1* %a
+  ret i16 %7
+}
+
+; Ensure that 1 is added to the high 20 bits if bit 11 of the low part is 1
+define i32 @lw_sw_constant(i32 %a) nounwind {
+; TODO: the addi should be folded in to the lw/sw
+; RV32I-LABEL: lw_sw_constant:
+; RV32I: lui a1, 912092
+; RV32I: addi a2, a1, -273
+; RV32I: lw a1, 0(a2)
+; RV32I: sw a0, 0(a2)
+; RV32I: addi a0, a1, 0
+  %1 = inttoptr i32 3735928559 to i32*
+  %2 = load volatile i32, i32* %1
+  store i32 %a, i32* %1
+  ret i32 %2
+}

test/CodeGen/RISCV/wide-mem.ll

+; RUN: llc -mtriple=riscv32 -verify-machineinstrs < %s \
+; RUN:   | FileCheck %s -check-prefix=RV32I
+
+; Check load/store operations on values wider than what is natively supported
+
+define i64 @load_i64(i64 *%a) nounwind {
+; RV32I-LABEL: load_i64:
+; RV32I: lw {{[a-z0-9]+}}, 0(a0)
+; RV32I: lw {{[a-z0-9]+}}, 4(a0)
+  %1 = load i64, i64* %a
+  ret i64 %1
+}