Page MenuHomePhabricator

[RISCV 15/n] Implement lowering of ISD::SELECT

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



There's nothing particular novel here, every backend without a conditional move has something similar to the custom inserter for SELECT_CC. Some backends try to use a mix of tablegen patterns and C++ code, for instance to deal with the condition code. I feel it's simpler when just handled in RISCVTargetLowering.

Although ISD::SELECT_CC is a more natural match for RISCVISD::SELECT_CC (and
ultimately the integer RISC-V conditional branch instructions), we choose to
expand ISD::SELECT_CC and lower ISD::SELECT. The appropriate compare+branch
will be created in the case where an ISD::SELECT condition value is created by
an ISD::SETCC node, which operates on XLen types. Other datatypes such as
floating point don't have conditional branch instructions, and lowering
ISD::SELECT allows more flexibility for handling these cases.

Diff Detail


Event Timeline

asb created this revision.Feb 14 2017, 5:47 AM
Razer6 added a subscriber: Razer6.Feb 14 2017, 6:28 AM
dylanmckay added inline comments.
101 ↗(On Diff #88360)

Over 80 char limit

asb updated this revision to Diff 112178.Aug 22 2017, 8:18 AM
asb marked an inline comment as done.

Update to fix line > 80 chars.

asb updated this revision to Diff 112454.Aug 23 2017, 2:27 PM

Instruction definition and patterns have been split, as discussed in

reames requested changes to this revision.Aug 24 2017, 5:24 PM
reames added a subscriber: reames.
reames added inline comments.
154 ↗(On Diff #112454)

A better name here would be more clear, maybe FallthroughMBB?

171 ↗(On Diff #112454)

Extracting this switch out as a helper function or lambda would aid readability. getBranchOpcodeForSetCCOpcode?

203 ↗(On Diff #112454)

Yuck. Please just use the names and don't update a variable in place. This is just confusing.

205 ↗(On Diff #112454)

Didn't you already do this above?

209 ↗(On Diff #112454)

Hm, as written, isn't this basically making a static prediction the true value is common? Is there profiling available on the SETCC instruction?

5 ↗(On Diff #112454)

Please expand your test to cover the whole diamond.

This revision now requires changes to proceed.Aug 24 2017, 5:24 PM
asb updated this revision to Diff 112687.Aug 25 2017, 6:20 AM
asb edited edge metadata.
asb marked 5 inline comments as done.

Hi Philip, many many thanks for the review - this code has been copied around several backends, and we can certainly do better (patches for those backends to follow!). This is exactly the sort of thing I'm trying to avoid with this backend project, so I really appreciate you calling it this case where the code fell short. The code comment was incorrect to say a diamond control flow pattern is being inserted, as it's actually just a triangle. Hopefully the Head/IfFalse/Tail names make more sense. I've made normaliseSetCC a separate helper function in addition to your suggested CondCode->BranchOpcode helper (this felt sensible, given getBranchOpcodeForIntCondCode expects its argument has been normalised in this way).

209 ↗(On Diff #112454)

I believe MachineBlockPlacement should handle this. RISC-V compilers should assume a forward branch is predicted not-taken, so the assumption would actually be the false path is the common case.

5 ↗(On Diff #112454)

The comment was incorrect, we're actually inserting a triangle (though as you point out, the confusing variable naming makes it really hard to see what is going on!)

Much, much cleaner. Thanks!

LGTM if you want to land as is, but you might consider waiting for one other review. I'm not super familiar with the tablegen and customer inserter stuff, so I may have missed something.

apazos added a subscriber: apazos.Aug 25 2017, 1:17 PM
apazos added inline comments.
89 ↗(On Diff #112687)

You might want to be consistent in code standard in the file and put the default case as the top one like done above.

asb updated this revision to Diff 112751.Aug 25 2017, 2:38 PM
asb marked an inline comment as done.

Update to put the 'default:' case first in a switch (thanks @apazos).

mgrang added a subscriber: mgrang.Sep 6 2017, 5:15 PM
mgrang added inline comments.
70 ↗(On Diff #112751)

Period at the end of comment.

189 ↗(On Diff #112751)


195 ↗(On Diff #112751)


asb updated this revision to Diff 114147.Sep 7 2017, 4:05 AM
asb marked 3 inline comments as done.

Full stops at end of comments.

psnobl added a subscriber: psnobl.Sep 8 2017, 12:10 PM
reames accepted this revision.Sep 10 2017, 7:18 PM

Reflecting previous LGTM again.

This revision is now accepted and ready to land.Sep 10 2017, 7:18 PM
asb updated this revision to Diff 122071.Nov 8 2017, 5:51 AM
asb retitled this revision from [RISCV 15/n] Implement lowering of ISD::SELECT_CC to [RISCV 15/n] Implement lowering of ISD::SELECT.
asb edited the summary of this revision. (Show Details)

This minor revision of the patch switches to lowering ISD::SELECT. This is motivated by future support for the floating point instruction set extensions and the coarse-grained nature of setOperationAction. See the updated patch description and new code comments for more discussion, but essentially we need to ensure that a RISC-V compare+branch instruction is generated whenever the comparison is between two XLEN integers.

johnrusso added inline comments.Nov 9 2017, 4:18 PM
173 ↗(On Diff #122071)

I noticed that the select-cc.ll test didn't cover this code.
This little snip with the correct matching pattern should cover it.

%val20 = select i1 %tst10, i32 %val18, i32 %val19

%tr3 = trunc i32 %val3 to i1
%val21 = select i1 %tr3, i32 %val20, i32 %val19

ret i32 %val21
188 ↗(On Diff #122071)

Could we use a 'switch(MI.getOpcode())' here and assert on an unhandled opcode if it occurs. I suspect other pseudos may require a custom inserter in the future.

asb updated this revision to Diff 122398.Nov 10 2017, 12:19 AM
asb marked an inline comment as done.

Adds a bare-select.ll test case, ensuring all code paths in lowerSELECT are tested.

173 ↗(On Diff #122071)

Well spotted, I've moved the bare-select.ll test from D29938 in to this patch to ensure the code path is exercised.

188 ↗(On Diff #122071)

Future patches move to doing this. For previous patches, reviewers have asked to not have single-element switches. I don't have a particularly strong view either way. The single-element switch minimises the diff in the future patches and is easier to extend, but has the disadvantage of looking a little odd.

This looks ready to merge. Can we have this in today?

This revision was automatically updated to reflect the committed changes.