[RISCV] Enable the LocalStackSlotAllocation pass support

[craig.topper]

Summary

For RISC-V, load/store(exclude vector load/store) instructions only has a 12 bit immediate operand. If the offset is out-of-range, it must make use of a temp register to make up this offset. If between these offsets, they have a small(IsInt<12>) relative offset, LocalStackSlotAllocation pass can find a value as frame base register's value, and replace the origin offset with this register's value plus the relative offset.

[craig.topper]

llvm/test/CodeGen/RISCV/local-stack-allocation.ll

1 ↗

(On Diff #328731)

Is it possible to pre-commit this test case and only show the changes here in this patch?

[craig.topper]

llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp

519

I think vector load/store can have a frameindex operand. There's no immediate field so it will get split out into an ADDI in eliminateFrameIndex, but I think that happens later.

[StephenFan]

llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp

519

In the RISCVIselDAGToDAG.cpp:

case ISD::FrameIndex: {
    SDValue Imm = CurDAG->getTargetConstant(0, DL, XLenVT);
    int FI = cast<FrameIndexSDNode>(Node)->getIndex();
    SDValue TFI = CurDAG->getTargetFrameIndex(FI, VT);
    ReplaceNode(Node, CurDAG->getMachineNode(RISCV::ADDI, DL, VT, TFI, Imm));
    return;
  }

So the llvm ir like this :

%addr = alloca i8
call void callee(i8* %addr)

the %addr will be selected to ADDI

[craig.topper]

llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp

519

Right. I was just asking why the comment says vector load/store are different?

[StephenFan]

llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp

519

Oh, sorry. You're right, the vector load/store can have a FrameIndex Operand. I made a mistake.

[StephenFan]

Pre-commit local-stack-allocation.ll test.

[StephenFan]

llvm/test/CodeGen/RISCV/local-stack-allocation.ll

17 ↗

(On Diff #328736)

This instruction will be eliminated in D92479 which I will land it soon after.

[StephenFan]

Fix comment error.

[StephenFan]

Ping.

[asb]

Could you please rebase this to account for D98716?

I _think_ the right fix is to change the call for needsStackRealignment to shouldRealignStack. But I'm seeing multiple runtime failures for the GCC torture suite with the patch applied. e.g. 20031012-1.c at O0 for rv32imafdc ilp32.

[StephenFan]

Fix error.

[StephenFan]

In D98101#2663432, @asb wrote:

Could you please rebase this to account for D98716?

I _think_ the right fix is to change the call for needsStackRealignment to shouldRealignStack. But I'm seeing multiple runtime failures for the GCC torture suite with the patch applied. e.g. 20031012-1.c at O0 for rv32imafdc ilp32.

Hi @asb . I wrote a script to compile and run the gcc c torture tests on qemu. According to the test result and as you said, multiple runtime failures appeared. However, 20031012-1.c at O0 for rv32imafdc ilp32 didn't fail on qemu. And I have fixed the failures and updated the patch. Can you help me testing the gcc c torture test again? Thanks!

[asb]

The GCC torture suite now gets a 100% pass rate for me - thanks for fixing. I've left various minor notes about comment phrasing and formatting etc.

I don't feel I've stepped through the logic of the patch carefully enough yet, but hopefully the suggested edits are actionable in the meantime.

llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp

501

No need to copy this doc comment TargetRegisterInfo.h.

508

returns => Returns

510

I think this sentence needs rewriting - I can't quite follow it.

515

clang-format prefers this with a space after the first ;

519

This thread is marked as done, but the comment still same to make the same claim that vector load/store don't have a frameindex?

538

I think: "the maximum possible offset relative to the frame pointer."?

544

bytes => byte and "maximum possible offset relative to the stack pointer." I think

560

Nit: end with full stop

579

Nit: re-wrap and end sentence in full stop.

589

Nit: end with full stop

597

Nit: end with full stop.

[StephenFan]

Address @asb 's comment

[StephenFan]

Delete "that" in comment

[StephenFan]

llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp

510

emm, This sentence is copied from TargetRegisterInfo.h. It means that, For RISCV, if a frame index operand has a Offset that out-of-range 12 bits, this function will return true to indicate this frame index operand needs a frame base register.

[StephenFan]

Full end

[StephenFan]

Ping.

[asb]

llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp

510

Yes that was my bad - the sentence does indeed make sense as written.

[luismarques]

I've benchmarked the impact of this patch with CoreMark and Embench and it looked good. In summary, there were no or minimal performance differences but there were various small improvements to code size, which are probably similar to your test case.
I was going to add other checks like running the llvm test suite but the patch no longer applies. Can you refresh it?

[jrtc27]

llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp

520–524

Can we not just look and see if it both has an FI and is an I or S type instruction? This isn't really flexible, and I don't see why it's needed either (and I'd have to more than double the size of this switch statement downstream in CHERI-LLVM to add both capability base and capability value instructions).

535–537

What about floating point registers? Can we also generalise this to iterate over the list of saved registers rather than copying the ABI to yet another place (and thus yet another place we'd have to change downstream in CHERI-LLVM)?

542

Why 128? A hard-coded constant common across all ISAs seems fishy.

582

We use the less-confusing name FIOperandNum elsewhere

584

The copies of this in RegisterScavenging and other backends have assert(i < MI.getNumOperands() && "Instr doesn't have FrameIndex operand!");

586–588

I don't see why the first part matters. Only include the relevant information, that "FrameIndex operands are always represented as a register followed by an immediate".

589

This variable seems pointless to me, just inline the +1

llvm/test/CodeGen/RISCV/local-stack-allocation.ll

30 ↗

(On Diff #338495)

Please fix this test, either before you pre-commit it or now as another pre-commit if you've already done so.

[craig.topper]

llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp

519

I think vector load/store no longer have frame indices.

520–524

This is missing LBU, LHU, and LWU.

I agree with @jrtc27 we should check the I or S type instead.

533

No need to say 'int' llvm almost always uses unsigned by itself.

535–537

Are X3 and X4 really callee saved to the stack? They're reserved so I don't think we ever spill them.

[StephenFan]

Address Comments.

[StephenFan]

Improve comments.

[StephenFan]

llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp

542

This value should be an experimental target-dependent value. But I don't know yet which value is appropriate for riscv.

[craig.topper]

llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp

535

Is ReservedByUser not already cover by getReservedRegs?

542

Can we copy the comments from ARM and AArch64 here including the FIXME for 128?

[StephenFan]

1. Remove check user reserved regsiter list.
2. Improve comments.

[craig.topper]

llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp

602

Should we assert that it is I or S format?

[StephenFan]

Add assertion.

[craig.topper]

I think we need this change from D134851 to match SVE from D83859

 bool isStackIdSafeForLocalArea(unsigned StackId) const override {
  // We don't support putting RVV objects into the pre-allocated local
  // frame block at the moment.
  return StackId != TargetStackID::ScalableVector;
}

[StephenFan]

Implement target hook.

[craig.topper]

llvm/test/CodeGen/RISCV/local-stack-slot-allocation.ll

7–8

Remove TODO

[StephenFan]

Remove TODO.

[StephenFan]

Ping :)

[craig.topper]

LGTM

[rogfer01]

I'm seeing failures in the llvm-testsuite on riscv64-unknown-linux-gnu with -O2 (no vectors) which git bisect attributes to this change.

Failed Tests (3):
  test-suite :: MultiSource/Benchmarks/DOE-ProxyApps-C++/CLAMR/CLAMR.test
  test-suite :: MultiSource/Benchmarks/tramp3d-v4/tramp3d-v4.test
  test-suite :: SingleSource/UnitTests/matrix-types-spec.test

Has anyone seen something similar?

[craig.topper]

In D98101#3883090, @rogfer01 wrote:

I'm seeing failures in the llvm-testsuite on riscv64-unknown-linux-gnu with -O2 (no vectors) which git bisect attributes to this change.

Failed Tests (3):
  test-suite :: MultiSource/Benchmarks/DOE-ProxyApps-C++/CLAMR/CLAMR.test
  test-suite :: MultiSource/Benchmarks/tramp3d-v4/tramp3d-v4.test
  test-suite :: SingleSource/UnitTests/matrix-types-spec.test

Has anyone seen something similar?

I think we're seeing those failures too.

[dnpetrov-sc]

This comment was removed by dnpetrov-sc.

[LiDongjin]

Fix the failures in the llvm-testsuite on riscv64-unknown-linux-gnu with -O2 (no vectors).

[LiDongjin]

For the failures in the llvm-testsuite, it seems some problems for Add instruction which generates wrong frame index.
Also in Arm and AArch64, No need to support for Add instruction.
Therefore load/store(exclude vector load/store) instruction is enough for LocalStackSlotAllocation pass.

[StephenFan]

If it would be better to add a test case that tests we don't do local stack slot allocation on ADDI instruction?

[craig.topper]

LGTM

[craig.topper]

Reverse ping. Can we commit this?

[rogfer01]

I'm seeing failures on the llvm testsuite after this change (scalar only).

Failed Tests (3):
  test-suite :: MultiSource/Benchmarks/DOE-ProxyApps-C++/CLAMR/CLAMR.test
  test-suite :: MultiSource/Benchmarks/tramp3d-v4/tramp3d-v4.test
  test-suite :: SingleSource/UnitTests/matrix-types-spec.test

Does anyone else see them too? Thanks!

[craig.topper]

In D98101#4013228, @rogfer01 wrote:

I'm seeing failures on the llvm testsuite after this change (scalar only).

Failed Tests (3):
  test-suite :: MultiSource/Benchmarks/DOE-ProxyApps-C++/CLAMR/CLAMR.test
  test-suite :: MultiSource/Benchmarks/tramp3d-v4/tramp3d-v4.test
  test-suite :: SingleSource/UnitTests/matrix-types-spec.test

Does anyone else see them too? Thanks!

Are those the exact same tests you reported last time?

[rogfer01]

In D98101#4013423, @craig.topper wrote:

In D98101#4013228, @rogfer01 wrote:

I'm seeing failures on the llvm testsuite after this change (scalar only).

Failed Tests (3):
  test-suite :: MultiSource/Benchmarks/DOE-ProxyApps-C++/CLAMR/CLAMR.test
  test-suite :: MultiSource/Benchmarks/tramp3d-v4/tramp3d-v4.test
  test-suite :: SingleSource/UnitTests/matrix-types-spec.test

Does anyone else see them too? Thanks!

Are those the exact same tests you reported last time?

Yes, I see the same failures. I admit I'm a bit puzzled.

Might be a problem on our side, so if someone can reproduce it too, that'd be helpful.

[craig.topper]

I've reverted the patch so I'm reopening.

[craig.topper]

It looks like maybe we're somehow creating load/stores with negative immediates that are supposed to be large positive immediates.

For example,
I see s9 pointing to sp+2152. A store that is supposed to be accessing sp+4288 was created with s9-1960. If we consider that -1960 as a uimm12 it would be +2136. 2152+2136 is the 4288 we were after.

This is backed up by running -verify-machineinstrs which generates a ton of errors about immediates that are uimm12 instead of simm12.

[craig.topper]

I think we failed to consider the offset in the store itself in RISCVRegisterInfo::isFrameOffsetLegal

[craig.topper]

Consider the instruction's local offset in isFrameOffsetLegal.

[LiDongjin]

modify the isFrameOffsetLegal to (Offset <= maxIntN(12)) && (Offset >= minIntN(12)),
Simply constrain offset to be within the range of 12-bit unsigned integers.

[craig.topper]

@LiDongjin I made changes and re-commited this already. Are you proposing additional changes?

[LiDongjin]

This comment was removed by LiDongjin.

[jrtc27]

In D98101#4032848, @LiDongjin wrote:

modify the isFrameOffsetLegal to (Offset <= maxIntN(12)) && (Offset >= minIntN(12)),

That’s what isIntN<12>(Offset) does

Simply constrain offset to be within the range of 12-bit unsigned integers.

Signed, not unsigned, which is already done

[LiDongjin]

Sorry I miss the message, I have no additional changes @craig.topper .

[jrtc27]

Can we please have the diff reverted back to what was committed and closed? This is a mess now…

[LiDongjin]

Just change the isFrameOffsetLegal like:

return (Offset <= maxIntN(12)) && (Offset >= minIntN(12));

[craig.topper]

Commandeering so I can fix the history

[craig.topper]

Well I guess I can't reclose this properly as long as @asb has a blocking review.

[craig.topper]

Restoring to commited version

[craig.topper]

Bumped @asb from reviewers to unblock.

This should be back to the version commited in 4554663bc0da71d61ab488641c95ef98430cb451

llvm/lib/Target/RISCV/RISCVFrameLowering.h

 //===-- RISCVFrameLowering.h - Define frame lowering for RISCV -*- C++ -*--===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
public:
   bool canUseAsPrologue(const MachineBasicBlock &MBB) const override;
   bool canUseAsEpilogue(const MachineBasicBlock &MBB) const override;
 
   bool enableShrinkWrapping(const MachineFunction &MF) const override;
 
   bool isSupportedStackID(TargetStackID::Value ID) const override;
   TargetStackID::Value getStackIDForScalableVectors() const override;
 
+  bool isStackIdSafeForLocalArea(unsigned StackId) const override {
+    // We don't support putting RISCV Vector objects into the pre-allocated
+    // local frame block at the moment.
+    return StackId != TargetStackID::ScalableVector;
+  }
+
 protected:
   const RISCVSubtarget &STI;
 
 private:
   void determineFrameLayout(MachineFunction &MF) const;
   void adjustStackForRVV(MachineFunction &MF, MachineBasicBlock &MBB,
                          MachineBasicBlock::iterator MBBI, const DebugLoc &DL,
                          int64_t Amount, MachineInstr::MIFlag Flag) const;
   std::pair<int64_t, Align>
   assignRVVStackObjectOffsets(MachineFunction &MF) const;
 };
 } // namespace llvm
 #endif

llvm/lib/Target/RISCV/RISCVRegisterInfo.h

 //===-- RISCVRegisterInfo.h - RISCV Register Information Impl ---*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
void adjustReg(MachineBasicBlock &MBB, MachineBasicBlock::iterator II,
                  const DebugLoc &DL, Register DestReg, Register SrcReg,
                  StackOffset Offset, MachineInstr::MIFlag Flag,
                  MaybeAlign RequiredAlign) const;
 
   bool eliminateFrameIndex(MachineBasicBlock::iterator MI, int SPAdj,
                            unsigned FIOperandNum,
                            RegScavenger *RS = nullptr) const override;
 
+  bool requiresVirtualBaseRegisters(const MachineFunction &MF) const override;
+
+  bool needsFrameBaseReg(MachineInstr *MI, int64_t Offset) const override;
+
+  bool isFrameOffsetLegal(const MachineInstr *MI, Register BaseReg,
+                          int64_t Offset) const override;
+
+  Register materializeFrameBaseRegister(MachineBasicBlock *MBB, int FrameIdx,
+                                        int64_t Offset) const override;
+
+  void resolveFrameIndex(MachineInstr &MI, Register BaseReg,
+                         int64_t Offset) const override;
+
+  int64_t getFrameIndexInstrOffset(const MachineInstr *MI,
+                                   int Idx) const override;
+
   void lowerVSPILL(MachineBasicBlock::iterator II) const;
   void lowerVRELOAD(MachineBasicBlock::iterator II) const;
 
   Register getFrameRegister(const MachineFunction &MF) const override;
 
   bool requiresRegisterScavenging(const MachineFunction &MF) const override {
     return true;
   }

llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp

 //===-- RISCVRegisterInfo.cpp - RISCV Register Information ------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
bool RISCVRegisterInfo::eliminateFrameIndex(MachineBasicBlock::iterator II,
   case RISCV::PseudoVRELOAD8_M1:
     lowerVRELOAD(II);
     return true;
   }
 
   return false;
 }
 
+bool RISCVRegisterInfo::requiresVirtualBaseRegisters(
+    const MachineFunction &MF) const {
+  return true;
+}
+
+// Returns true if the instruction's frame index reference would be better
+// served by a base register other than FP or SP.
+// Used by LocalStackSlotAllocation pass to determine which frame index
+// references it should create new base registers for.
+bool RISCVRegisterInfo::needsFrameBaseReg(MachineInstr *MI,
+                                          int64_t Offset) const {
+  unsigned FIOperandNum = 0;
+  for (; !MI->getOperand(FIOperandNum).isFI(); FIOperandNum++)
+    assert(FIOperandNum < MI->getNumOperands() &&
+           "Instr doesn't have FrameIndex operand");
+
+  // For RISC-V, The machine instructions that include a FrameIndex operand
+  // are load/store, ADDI instructions.
+  unsigned MIFrm = RISCVII::getFormat(MI->getDesc().TSFlags);
+  if (MIFrm != RISCVII::InstFormatI && MIFrm != RISCVII::InstFormatS)
+    return false;
+  // We only generate virtual base registers for loads and stores, so
+  // return false for everything else.
+  if (!MI->mayLoad() && !MI->mayStore())
+    return false;
+
+  const MachineFunction &MF = *MI->getMF();
+  const MachineFrameInfo &MFI = MF.getFrameInfo();
+  const RISCVFrameLowering *TFI = getFrameLowering(MF);
+  const MachineRegisterInfo &MRI = MF.getRegInfo();
+  unsigned CalleeSavedSize = 0;
+  Offset += getFrameIndexInstrOffset(MI, FIOperandNum);
+
+  // Estimate the stack size used to store callee saved registers(
+  // excludes reserved registers).
+  BitVector ReservedRegs = getReservedRegs(MF);
+  for (const MCPhysReg *R = MRI.getCalleeSavedRegs(); MCPhysReg Reg = *R; ++R) {
+    if (!ReservedRegs.test(Reg))
+      CalleeSavedSize += getSpillSize(*getMinimalPhysRegClass(Reg));
+  }
+
+  int64_t MaxFPOffset = Offset - CalleeSavedSize;
+  if (TFI->hasFP(MF) && !shouldRealignStack(MF))
+    return !isFrameOffsetLegal(MI, RISCV::X8, MaxFPOffset);
+
+  // Assume 128 bytes spill slots size to estimate the maximum possible
+  // offset relative to the stack pointer.
+  // FIXME: The 128 is copied from ARM. We should run some statistics and pick a
+  // real one for RISC-V.
+  int64_t MaxSPOffset = Offset + 128;
+  MaxSPOffset += MFI.getLocalFrameSize();
+  return !isFrameOffsetLegal(MI, RISCV::X2, MaxSPOffset);
+}
+
+// Determine whether a given base register plus offset immediate is
+// encodable to resolve a frame index.
+bool RISCVRegisterInfo::isFrameOffsetLegal(const MachineInstr *MI,
+                                           Register BaseReg,
+                                           int64_t Offset) const {
+  unsigned FIOperandNum = 0;
+  while (!MI->getOperand(FIOperandNum).isFI()) {
+    FIOperandNum++;
+    assert(FIOperandNum < MI->getNumOperands() &&
+           "Instr does not have a FrameIndex operand!");
+  }
+
+  Offset += getFrameIndexInstrOffset(MI, FIOperandNum);
+  return isInt<12>(Offset);
+}
+
+// Insert defining instruction(s) for a pointer to FrameIdx before
+// insertion point I.
+// Return materialized frame pointer.
+Register RISCVRegisterInfo::materializeFrameBaseRegister(MachineBasicBlock *MBB,
+                                                         int FrameIdx,
+                                                         int64_t Offset) const {
+  MachineBasicBlock::iterator MBBI = MBB->begin();
+  DebugLoc DL;
+  if (MBBI != MBB->end())
+    DL = MBBI->getDebugLoc();
+  MachineFunction *MF = MBB->getParent();
+  MachineRegisterInfo &MFI = MF->getRegInfo();
+  const TargetInstrInfo *TII = MF->getSubtarget().getInstrInfo();
+
+  Register BaseReg = MFI.createVirtualRegister(&RISCV::GPRRegClass);
+  BuildMI(*MBB, MBBI, DL, TII->get(RISCV::ADDI), BaseReg)
+      .addFrameIndex(FrameIdx)
+      .addImm(Offset);
+  return BaseReg;
+}
+
+// Resolve a frame index operand of an instruction to reference the
+// indicated base register plus offset instead.
+void RISCVRegisterInfo::resolveFrameIndex(MachineInstr &MI, Register BaseReg,
+                                          int64_t Offset) const {
+  unsigned FIOperandNum = 0;
+  while (!MI.getOperand(FIOperandNum).isFI()) {
+    FIOperandNum++;
+    assert(FIOperandNum < MI.getNumOperands() &&
+           "Instr does not have a FrameIndex operand!");
+  }
+
+  Offset += getFrameIndexInstrOffset(&MI, FIOperandNum);
+  // FrameIndex Operands are always represented as a
+  // register followed by an immediate.
+  MI.getOperand(FIOperandNum).ChangeToRegister(BaseReg, false);
+  MI.getOperand(FIOperandNum + 1).ChangeToImmediate(Offset);
+}
+
+// Get the offset from the referenced frame index in the instruction,
+// if there is one.
+int64_t RISCVRegisterInfo::getFrameIndexInstrOffset(const MachineInstr *MI,
+                                                    int Idx) const {
+  assert((RISCVII::getFormat(MI->getDesc().TSFlags) == RISCVII::InstFormatI ||
+          RISCVII::getFormat(MI->getDesc().TSFlags) == RISCVII::InstFormatS) &&
+         "The MI must be I or S format.");
+  assert(MI->getOperand(Idx).isFI() && "The Idx'th operand of MI is not a "
+                                       "FrameIndex operand");
+  return MI->getOperand(Idx + 1).getImm();
+}
+
 Register RISCVRegisterInfo::getFrameRegister(const MachineFunction &MF) const {
   const TargetFrameLowering *TFI = getFrameLowering(MF);
   return TFI->hasFP(MF) ? RISCV::X8 : RISCV::X2;
 }
 
 const uint32_t *
 RISCVRegisterInfo::getCallPreservedMask(const MachineFunction & MF,
                                         CallingConv::ID CC) const {

llvm/test/CodeGen/RISCV/local-stack-slot-allocation.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc -mtriple=riscv32 -verify-machineinstrs < %s | FileCheck %s --check-prefix=RV32I
 ; RUN: llc -mtriple=riscv64 -verify-machineinstrs < %s | FileCheck %s --check-prefix=RV64I
 
 ; This test case test the LocalStackSlotAllocation pass that use a base register
 ; for the frame index that its offset is out-of-range (for RISC-V. the immediate
 ; is 12 bits for the load store instruction (excludes vector load / store))
-; TODO: Enable LocalStackSlotAllocation pass.
 define void @use_frame_base_reg() {
 ; RV32I-LABEL: use_frame_base_reg:
 ; RV32I:       # %bb.0:
 ; RV32I-NEXT:    lui a0, 24
 ; RV32I-NEXT:    addi a0, a0, 1712
 ; RV32I-NEXT:    sub sp, sp, a0
 ; RV32I-NEXT:    .cfi_def_cfa_offset 100016
 ; RV32I-NEXT:    lui a0, 24
+; RV32I-NEXT:    addi a0, a0, 1704
 ; RV32I-NEXT:    add a0, sp, a0
-; RV32I-NEXT:    lb a0, 1708(a0)
-; RV32I-NEXT:    lui a0, 24
-; RV32I-NEXT:    add a0, sp, a0
-; RV32I-NEXT:    lb a0, 1704(a0)
+; RV32I-NEXT:    lb a1, 4(a0)
+; RV32I-NEXT:    lb a0, 0(a0)
 ; RV32I-NEXT:    lui a0, 24
 ; RV32I-NEXT:    addi a0, a0, 1712
 ; RV32I-NEXT:    add sp, sp, a0
 ; RV32I-NEXT:    ret
 ;
 ; RV64I-LABEL: use_frame_base_reg:
 ; RV64I:       # %bb.0:
 ; RV64I-NEXT:    lui a0, 24
 ; RV64I-NEXT:    addiw a0, a0, 1712
 ; RV64I-NEXT:    sub sp, sp, a0
 ; RV64I-NEXT:    .cfi_def_cfa_offset 100016
 ; RV64I-NEXT:    lui a0, 24
+; RV64I-NEXT:    addiw a0, a0, 1704
 ; RV64I-NEXT:    add a0, sp, a0
-; RV64I-NEXT:    lb a0, 1708(a0)
-; RV64I-NEXT:    lui a0, 24
-; RV64I-NEXT:    add a0, sp, a0
-; RV64I-NEXT:    lb a0, 1704(a0)
+; RV64I-NEXT:    lb a1, 4(a0)
+; RV64I-NEXT:    lb a0, 0(a0)
 ; RV64I-NEXT:    lui a0, 24
 ; RV64I-NEXT:    addiw a0, a0, 1712
 ; RV64I-NEXT:    add sp, sp, a0
 ; RV64I-NEXT:    ret
 
   %va = alloca i8, align 4
   %va1 = alloca i8, align 4
   %large = alloca [ 100000 x i8 ]
   %argp.cur = load volatile i8, ptr %va, align 4
   %argp.next = load volatile i8, ptr %va1, align 4
   ret void
 }
+
+; Test containing a load with its own local offset. Make sure isFrameOffsetLegal
+; considers it and does not create a virtual base register.
+define void @load_with_offset() {
+; RV32I-LABEL: load_with_offset:
+; RV32I:       # %bb.0:
+; RV32I-NEXT:    lui a0, 25
+; RV32I-NEXT:    addi a0, a0, -1792
+; RV32I-NEXT:    sub sp, sp, a0
+; RV32I-NEXT:    .cfi_def_cfa_offset 100608
+; RV32I-NEXT:    lui a0, 25
+; RV32I-NEXT:    add a0, sp, a0
+; RV32I-NEXT:    lb a0, -292(a0)
+; RV32I-NEXT:    lui a0, 24
+; RV32I-NEXT:    add a0, sp, a0
+; RV32I-NEXT:    lb a0, 1704(a0)
+; RV32I-NEXT:    lui a0, 25
+; RV32I-NEXT:    addi a0, a0, -1792
+; RV32I-NEXT:    add sp, sp, a0
+; RV32I-NEXT:    ret
+;
+; RV64I-LABEL: load_with_offset:
+; RV64I:       # %bb.0:
+; RV64I-NEXT:    lui a0, 25
+; RV64I-NEXT:    addiw a0, a0, -1792
+; RV64I-NEXT:    sub sp, sp, a0
+; RV64I-NEXT:    .cfi_def_cfa_offset 100608
+; RV64I-NEXT:    lui a0, 25
+; RV64I-NEXT:    add a0, sp, a0
+; RV64I-NEXT:    lb a0, -292(a0)
+; RV64I-NEXT:    lui a0, 24
+; RV64I-NEXT:    add a0, sp, a0
+; RV64I-NEXT:    lb a0, 1704(a0)
+; RV64I-NEXT:    lui a0, 25
+; RV64I-NEXT:    addiw a0, a0, -1792
+; RV64I-NEXT:    add sp, sp, a0
+; RV64I-NEXT:    ret
+
+  %va = alloca [100 x i8], align 4
+  %va1 = alloca [500 x i8], align 4
+  %large = alloca [100000 x i8]
+  %va_gep = getelementptr [100 x i8], ptr %va, i64 16
+  %va1_gep = getelementptr [100 x i8], ptr %va1, i64 0
+  %load = load volatile i8, ptr %va_gep, align 4
+  %load1 = load volatile i8, ptr %va1_gep, align 4
+  ret void
+}