This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Add a pass to do block predication on SSA machine IR
ClosedPublic

Authored by ThomasRaoux on Aug 13 2019, 5:31 PM.

Details

Reviewers
jmolloy
majnemer
Summary

For targets requiring aggressive scheduling and/or software pipeline we need to apply predication before preRA scheduling. This adds a pass re-using the early if-cvt infrastructure but generating predicated instructions instead of speculatively executing instructions. It allows doing if conversion on blocks containing instructions with side-effects. The pass re-use the target hook from postRA if-conversion to let the target decide on the heuristic to apply.

Diff Detail

Event Timeline

ThomasRaoux created this revision.Aug 13 2019, 5:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 13 2019, 5:31 PM

Tests missing; please upload all patches with full context (-U99999)

Tests missing; please upload all patches with full context (-U99999)

Updated the patch to have full context. Sorry about that. As mentioned in the description there are currently no target in LLVM trunk supporting both predicate and select instructions hooks and this is needed for this pass to work. So I'm not able to add tests at the moment and the target I'm working in cannot be up-streamed right now. If this is not acceptable I'll keep this in my target for now.

One more question - i can't recall if there's precedent for bundling more than one pass
into the same source file, that does not look ideal.

Can the pass not be run standalone, does it really require some specific target?
Perhaps that can be short-circuited via some cl::opt just for testing purposes?

I'm not sure it's a great idea to add dead code with no upstream test coverage.
At best, it will silently regress, at worst it can simply be dropped as dead code.

llvm/lib/CodeGen/EarlyIfConversion.cpp
931–933

I'd think you want to not jam it next to existing pass, but put it into it's own place

One more question - i can't recall if there's precedent for bundling more than one pass
into the same source file, that does not look ideal.

Looking quickly at it, I see few cases (ex: https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/MachineInstrBundle.cpp) but doesn't mean it is a great idea.
The alternative would be to move the SSAIfConv helper class in a header file. Do you think this is a better solution?

Can the pass not be run standalone, does it really require some specific target?
Perhaps that can be short-circuited via some cl::opt just for testing purposes?

It can be ran standalone but unless the target has support for predicate and implements the canInsertSelect hook it won't do anything. It needs both features to do the transformation.

I'm not sure it's a great idea to add dead code with no upstream test coverage.
At best, it will silently regress, at worst it can simply be dropped as dead code.

I agree. If moving the SSAIfConv as a helper that can be reused is an acceptable solution, then I can do that and then it is easy to move the pass as is. Otherwise I'll probably copy this code for now until we have a target supporting the required features.

I figured the functionality can be tested with Hexagon backend, so I added a test in Hexagon target to test the predication.

majnemer added inline comments.Aug 14 2019, 5:12 PM
llvm/lib/CodeGen/EarlyIfConversion.cpp
472–473

Update this comment for the new logic.

988

Range based for loop?

1005–1006

Ditto.

ThomasRaoux marked 4 inline comments as done.
ThomasRaoux added inline comments.
llvm/lib/CodeGen/EarlyIfConversion.cpp
931–933

Do you mean move it into its own file? This reuses SSAIfConv infrastructure so unless we move it into a header file those two passes have to be in the same file.

lebedev.ri added inline comments.Aug 15 2019, 12:03 AM
llvm/lib/CodeGen/EarlyIfConversion.cpp
931–933

Yes

ThomasRaoux marked an inline comment as done.Aug 15 2019, 12:41 AM
ThomasRaoux added inline comments.
llvm/lib/CodeGen/EarlyIfConversion.cpp
931–933

So you are suggesting moving SSAIfConv into a common header? I don't have a strong opinion about it but there are several cases of passes sharing a file and I suppose limiting exposure of the helper has its advantage.

lebedev.ri added inline comments.Aug 15 2019, 12:53 AM
llvm/lib/CodeGen/EarlyIfConversion.cpp
931–933

So you are suggesting moving SSAIfConv into a common header?

Yes.
I'm also wondering what would be the best structure for that class,
should it do both things, or should it be generalized somehow,
and have two classes that inherit form it (to cmov, to branches)
that do the actual work.

jmolloy added inline comments.Aug 15 2019, 9:26 AM
llvm/lib/CodeGen/EarlyIfConversion.cpp
296

I'm not sure we should be so concerned with this case for predication, but it's sensible to keep this in for now. Erring on the side of caution is good.

323

s/predicatable/predicable/

339

nit: General LLVM style is to elide braces around single statements.

637

nit: /*ReversePredicate=*/false (add =, remove space, for standard comment style that clang-tidy can pick up).

931–933

FWIW I don't think this should be a concern. Moving code for the sake of moving code doesn't sound particularly useful to me. I see this as analogous to MachineScheduler and PostMachineScheduler which share common infrastructure and also the same file.

I'm also wondering what would be the best structure for that class

This current structure looks fine to me. Roman, if you have suggestions for how you'd like the code to look, perhaps you could mention them explicitly? your current comment doesn't feel particularly actionable.

972

typo.

1057

For my own gratification, could you please comment (in the review, not the code) why this is required for predication but not for speculation?

ThomasRaoux marked 7 inline comments as done.
ThomasRaoux added inline comments.
llvm/lib/CodeGen/EarlyIfConversion.cpp
296

Agree.

1057

It is required for both. Those pass preserve the dominator and loop analysis.

jmolloy accepted this revision.Aug 16 2019, 9:58 AM

Hi Thomas,

Thanks for this! one comment but overall LGTM.

James

llvm/lib/CodeGen/EarlyIfConversion.cpp
983

This looks stateless with respect to DomTree and Removed; we should share this between the two implementations.

This revision is now accepted and ready to land.Aug 16 2019, 9:58 AM
ThomasRaoux marked an inline comment as done.
ThomasRaoux added inline comments.
llvm/lib/CodeGen/EarlyIfConversion.cpp
983

Fixed it by moving it to functions.

ThomasRaoux edited the summary of this revision. (Show Details)Aug 19 2019, 11:06 AM
ThomasRaoux closed this revision.Sep 2 2019, 9:34 PM