This is an archive of the discontinued LLVM Phabricator instance.

[CodeGenPrepare] Sink and duplicate more 'and' instructions.
ClosedPublic

Authored by gberry on Jan 17 2017, 10:14 AM.

Details

Summary

Rework the code that was sinking/duplicating (icmp and, 0) sequences
into blocks where they were being used by conditional branches to form
more tbz instructions on AArch64. The new code is more general in that
it just looks for 'and's that have all icmp 0's as users, with a target
hook used to select which subset of 'and' instructions to consider.
This change also enables 'and' sinking for X86, where it is more widely
beneficial than on AArch64.

The 'and' sinking/duplicating code is moved into the optimizeInst phase
of CodeGenPrepare, where it can take advantage of the fact the
OptimizeCmpExpression has already sunk/duplicated any icmps into the
blocks where they are used. One minor complication from this change is
that optimizeLoadExt needed to be updated to always mark 'and's it has
determined should be in the same block as their feeding load in the
InsertedInsts set to avoid an infinite loop of hoisting and sinking the
same 'and'.

This change fixes a regression on X86 in the tsan runtime caused by
moving GVNHoist to a later place in the optimization pipeline (see
PR31382).

Diff Detail

Repository
rL LLVM

Event Timeline

gberry created this revision.Jan 17 2017, 10:14 AM
mcrosier added inline comments.Jan 17 2017, 2:33 PM
lib/CodeGen/CodeGenPrepare.cpp
1105 ↗(On Diff #84696)

minor nit: Would it make sense for this check to be first? That would avoid unnecessary work for those targets that don't enable this optimization.

1112 ↗(On Diff #84696)

dyn_cast -> isa

4699 ↗(On Diff #84696)

I'm curious if this change in isolation prevented some other optimizations from happening?

MatzeB edited edge metadata.

Looks good to me, but it would probably be best to get the opinion of someone more active on the X86 target.

Looks good to me, but it would probably be best to get the opinion of someone more active on the X86 target.

LGTM as well assuming no additional feedback from the X86 developers.

RKSimon added inline comments.Jan 18 2017, 9:51 AM
test/CodeGen/X86/and-sink.ll
1 ↗(On Diff #84696)

Don't use -march - use -mtriple=i686-unknown

gberry marked 2 inline comments as done.Jan 19 2017, 2:44 PM
gberry added inline comments.
lib/CodeGen/CodeGenPrepare.cpp
1105 ↗(On Diff #84696)

Looking at this again, I'm tempted to move it down past the users loop since it is a virtual function call that is currently being made for nearly all 'and' instructions.

4699 ↗(On Diff #84696)

It shouldn't but I can verify that it doesn't.

test/CodeGen/X86/and-sink.ll
1 ↗(On Diff #84696)

Will do. Out of curiosity, why is this preferred?

RKSimon added inline comments.Jan 19 2017, 2:57 PM
test/CodeGen/X86/and-sink.ll
1 ↗(On Diff #84696)

Without a specific triple it will default to the buildbot's, tests often fail on windows buildbots or other targets with ABIs different enough to cause changes in register usage or scheduling.

gberry marked an inline comment as done.Jan 19 2017, 4:54 PM
gberry added inline comments.
lib/CodeGen/CodeGenPrepare.cpp
4699 ↗(On Diff #84696)

Verified that this change (and above related one) make no difference to generated code across all of our tests.

qcolombet edited edge metadata.Jan 25 2017, 11:18 AM

Hi,

Given this is CGP, I would like to see opt -codegenprepare checks as well.

Cheers,
-Quentin

lib/CodeGen/CodeGenPrepare.cpp
1091 ↗(On Diff #84696)

Could you add an assert for !InsertedInsts.count(AndI)

gberry updated this revision to Diff 85796.Jan 25 2017, 1:35 PM

Update based on Quentin's comments:
add opt -codegenprepare tests cases
add InsertedInsts assert in sinkAndCmp0Expression

RKSimon resigned from this revision.Jan 26 2017, 1:04 AM
gberry added a comment.Feb 6 2017, 8:49 AM

Ping? Any active X86 backend developers care to comment on this?

craig.topper edited edge metadata.Feb 15 2017, 9:22 PM

I think the X86 changes seem good.

qcolombet added inline comments.Feb 16 2017, 6:09 PM
test/Transforms/CodeGenPrepare/AArch64/and-sink.ll
1 ↗(On Diff #85796)

I would have expecting this to be in the same file as the CodeGen test, just because if we add a test for there, we would have to replicate it here.

gberry updated this revision to Diff 88933.Feb 17 2017, 11:35 AM

Put llc and opt -codegenprepare lit tests in the same file since they use the same test IR

gberry marked an inline comment as done.Feb 17 2017, 11:49 AM
This revision is now accepted and ready to land.Feb 17 2017, 3:08 PM
This revision was automatically updated to reflect the committed changes.