Page MenuHomePhabricator

[RISCV 13/n] Codegen for conditional branches
ClosedPublic

Authored by asb on Feb 14 2017, 5:32 AM.

Details

Summary

A good portion of this patch is the extra functions that needed to be implemented to support the test case. e.g. storeRegToStackSlot, loadRegFromStackSlot, eliminateFrameIndex.

Setting ISD::BR_CC to Expand may appear non-obvious on an architecture with branch+cmp instructions. However, I found it much easier to deal with matching the expanded form.

I had to change simm13_lsb0 and simm21_lsb0 to use the Operand<OtherVT> class rather than Operand<i32> in order to keep tablegen happy. This isn't a big deal, but it does seem a shame to lose the uniformity across immediate types when there's not an obvious benefit (I'm hoping a tablegen expert will educate me on what I'm missing here!).

Diff Detail

Repository
rL LLVM

Event Timeline

asb created this revision.Feb 14 2017, 5:32 AM
Razer6 added a subscriber: Razer6.Feb 14 2017, 6:28 AM
asb updated this revision to Diff 111631.Aug 18 2017, 1:52 AM

Update after auditing for inappropriate use of llvm_unreachable. Replace these uses with report_fatal_error.

dylanmckay added inline comments.Aug 21 2017, 6:14 AM
lib/Target/RISCV/RISCV.h
26 ↗(On Diff #111631)

Can we make these forward declarations alphabetically ordered?

lib/Target/RISCV/RISCVInstrInfo.h
39 ↗(On Diff #111631)

IsKill should start with a capital letter

lib/Target/RISCV/RISCVRegisterInfo.cpp
59 ↗(On Diff #111631)

Can we make this assertion message better?

72 ↗(On Diff #111631)

This sounds less like a fatal error and more like an assertion

80 ↗(On Diff #111631)
asb updated this revision to Diff 111970.Aug 21 2017, 6:49 AM
asb marked 5 inline comments as done.

Updated to address review comments from @dylanmckay (thanks!).

dylanmckay edited edge metadata.Aug 21 2017, 7:45 AM

Are there any tests for the stack slot loading/storing? If there are some in a later patch, that's fine too.

lib/Target/RISCV/RISCVRegisterInfo.cpp
78 ↗(On Diff #111970)

I think it makes more sense for report_fatal_error to live outside of the else entirely, and then the else can be removed as well.

asb added a comment.Aug 21 2017, 8:06 AM

Are there any tests for the stack slot loading/storing? If there are some in a later patch, that's fine too.

They come later. There should really be a comment in eliminateFrameIndex to indicate that it is a placeholder implementation to allow codegen for conditional branches to be testable.

lib/Target/RISCV/RISCVRegisterInfo.cpp
78 ↗(On Diff #111970)

Just to clarify, are you suggesting the following?:

if (!isInt<12>(Offset)) {
  report_fatal_error(...)
}

MI.getOperand(FIOperandNum).ChangeToRegister(FrameReg, false);
MI.getOperand(FIOperandNum + 1).ChangeToImmediate(Offset);
return;

LGTM with minor error-handling nitpick

lib/Target/RISCV/RISCVRegisterInfo.cpp
78 ↗(On Diff #111970)

I was thinking something more like the following

cpp
  if (isInt<12>(Offset)) {
    MI.getOperand(FIOperandNum).ChangeToRegister(FrameReg, false);
    MI.getOperand(FIOperandNum + 1).ChangeToImmediate(Offset);
  }

  report_fatal_error("Frame offsets outside of the signed 12-bit range not supported");
  return;

But now that you mention it, handling the error case early like your mockup seems better

asb updated this revision to Diff 112177.Aug 22 2017, 7:57 AM
asb marked 3 inline comments as done.

Update to address review comments. Thanks again @dylanmckay. If you're happy with this, please mark it accepted in Phabricator (makes it a little easier for seeing the current status of the RISC-V patchset at a glance).

dylanmckay accepted this revision.Aug 22 2017, 9:48 PM
This revision is now accepted and ready to land.Aug 22 2017, 9:48 PM
asb updated this revision to Diff 112450.Aug 23 2017, 2:24 PM

Instruction definitions and patterns have been split, as discussed in http://lists.llvm.org/pipermail/llvm-dev/2017-August/116635.html. There is no functional change vs the accepted patch revision.

apazos added a subscriber: apazos.Aug 25 2017, 2:00 PM
apazos added inline comments.
lib/Target/RISCV/RISCVInstrInfo.td
63 ↗(On Diff #112450)

Maybe these td file changes should move to a separate patch?

asb added inline comments.Aug 25 2017, 2:33 PM
lib/Target/RISCV/RISCVInstrInfo.td
63 ↗(On Diff #112450)

I think they belong here, as the change is made necessary by the codegen implementation. Or perhaps I'm misunderstanding your concern?

psnobl added a subscriber: psnobl.Sep 8 2017, 12:10 PM
This revision was automatically updated to reflect the committed changes.
sabuasal added inline comments.
llvm/trunk/lib/Target/RISCV/RISCVInstrInfo.td
70

Hi Alex,

I have two questions about this:

  1. Why don't we have this type inherting from ImmLeaf?
  2. Why is this not using XLenVT?

I am trying to add ImmLeaf since I need them for some checks for that type but it is causing a crash while running table gen about some type compatibility ("Type set is empty for each HW mode")

asb added a subscriber: kparzysz.Jan 22 2018, 2:06 AM
asb added inline comments.
llvm/trunk/lib/Target/RISCV/RISCVInstrInfo.td
70

As you've noted, it doesn't work when defined as an immediate type. I noted this in the patch description for this review, but nobody seems to have had any ideas on retaining def simm21_lsb0 : Operand<XLenVT>. Ultimately, branch operands are always symbols during codegen rather than immediates. Every other backend I checked ended up using Operand<OtherVT> for branch targets which at least implies I'm not missing something obvious. I think ideally we'd be able to specify that symbol operands are coerced to XLenVT.

If there's no cleaner solution, I suppose you could define a simm13_lsb0pred and use that in pattern definitions for instruction compression transformations. I don't like that we lose consistency with other operand definitions of course...

@kparzysz may be able to give more insight into the TableGen type check error.