This is an archive of the discontinued LLVM Phabricator instance.

[SelectionDAG][X86][WIP] Don't always seperate conditions in `(br (and/or cond0, cond1))` into seperate branches
Needs ReviewPublic

Authored by goldstein.w.n on Aug 9 2023, 9:03 PM.

Details

Reviewers
pengfei
RKSimon
Summary

It makes sense to split if the cost of computing cond1 is high
(proportionally to how likely cond0 is), but it doesn't really make
sense to introduce a second branch if its only a few instructions.

Splitting can also get in the way of potentially folding patterns.

This patch introduces some logic to try to check if the cost of
computing cond1 is relatively low, and if so don't split the
branches.

Note: The cost-model aspect of this patch is entirely not
benchmarked. This is a WIP and at the very least needs tuning.

Diff Detail

Event Timeline

goldstein.w.n created this revision.Aug 9 2023, 9:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2023, 9:03 PM
Herald added a subscriber: hiraditya. · View Herald Transcript
goldstein.w.n requested review of this revision.Aug 9 2023, 9:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2023, 9:03 PM

I have wondered in the past if this should be done by codegenprepare instead of SelectionDAGBuilder. I believe codegenprepare does it for fast isel.

The current code in SelectionDAGBuilder doesn’t sink an AND so it blocks forming TEST instructions. CodegenPrepare already knows how to sink AND to be near compare.

I have wondered in the past if this should be done by codegenprepare instead of SelectionDAGBuilder. I believe codegenprepare does it for fast isel.

The current code in SelectionDAGBuilder doesn’t sink an AND so it blocks forming TEST instructions. CodegenPrepare already knows how to sink AND to be near compare.

Is in break up the branches in IR instead then let selectiondagbuilder just translate 1-1?
I see splitBranchCondition in CodeGenPrepare. You'd suggest there?

I have wondered in the past if this should be done by codegenprepare instead of SelectionDAGBuilder. I believe codegenprepare does it for fast isel.

The current code in SelectionDAGBuilder doesn’t sink an AND so it blocks forming TEST instructions. CodegenPrepare already knows how to sink AND to be near compare.

Is in break up the branches in IR instead then let selectiondagbuilder just translate 1-1?
I see splitBranchCondition in CodeGenPrepare. You'd suggest there?

Yep that looks like the code. I see there is a FIXME there to remove the SelectionDAGBuilder code. If I remember right CodeGenPrepare creates the branches in a different order than SelectionDAGBuilder does. Whether that's good or bad I don't know, just that it's different.

I have wondered in the past if this should be done by codegenprepare instead of SelectionDAGBuilder. I believe codegenprepare does it for fast isel.

The current code in SelectionDAGBuilder doesn’t sink an AND so it blocks forming TEST instructions. CodegenPrepare already knows how to sink AND to be near compare.

Is in break up the branches in IR instead then let selectiondagbuilder just translate 1-1?
I see splitBranchCondition in CodeGenPrepare. You'd suggest there?

Yep that looks like the code. I see there is a FIXME there to remove the SelectionDAGBuilder code. If I remember right CodeGenPrepare creates the branches in a different order than SelectionDAGBuilder does. Whether that's good or bad I don't know, just that it's different.

Got it. Thanks for the help!

I'll port splitting logic (probably two patches including the one)

goldstein.w.n added a comment.EditedAug 11 2023, 2:45 PM

I have wondered in the past if this should be done by codegenprepare instead of SelectionDAGBuilder. I believe codegenprepare does it for fast isel.

The current code in SelectionDAGBuilder doesn’t sink an AND so it blocks forming TEST instructions. CodegenPrepare already knows how to sink AND to be near compare.

Is in break up the branches in IR instead then let selectiondagbuilder just translate 1-1?
I see splitBranchCondition in CodeGenPrepare. You'd suggest there?

Yep that looks like the code. I see there is a FIXME there to remove the SelectionDAGBuilder code. If I remember right CodeGenPrepare creates the branches in a different order than SelectionDAGBuilder does. Whether that's good or bad I don't know, just that it's different.

So the splitting logic in CodeGenPrepare seems to only be valid under TM->Options.EnableFastISel, otherwise starting running into:

llvm/Analysis/BlockFrequencyInfoImpl.h:1800: void llvm::BlockFrequencyInfoImpl<llvm::BasicBlock>::verifyMatch(BlockFrequencyInfoImpl<BT> &) const [BlockT = llvm::BasicBlock]: Assertion `Match && "BFI mismatch"' failed.

Given the constraint, porting all the logic seems to have meaningful impact.

Maybe keep in SelectionDAGBuilder?