Page MenuHomePhabricator

[RISCV 13/n] Codegen for conditional branches

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



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


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
26 ↗(On Diff #111631)

Can we make these forward declarations alphabetically ordered?

39 ↗(On Diff #111631)

IsKill should start with a capital letter

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.

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.

78 ↗(On Diff #111970)

Just to clarify, are you suggesting the following?:

if (!isInt<12>(Offset)) {

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

LGTM with minor error-handling nitpick

78 ↗(On Diff #111970)

I was thinking something more like the following

  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");

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 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.
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
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.

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.

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.