Page MenuHomePhabricator

[InstCombine] Simplify phis with incoming pointer-casts.
ClosedPublic

Authored by fhahn on Mar 5 2021, 10:04 AM.

Details

Summary

If the incoming values of a phi are pointer casts of the same original
value, replace the phi with a single cast. Such redundant phis are
somewhat common after loop-rotate and removing them can avoid some
unnecessary code bloat, e.g. because an iteration of a loop is peeled
off to make the phi invariant. It should also simplify further analysis
on its own.

InstCombine already uses stripPointerCasts in a couple of places and
also simplifies phis based on the incoming values, so the patch should
fit in the existing scope.

The patch causes binary changes in 47 out of 237 benchmarks in
MultiSource/SPEC2000/SPEC2006 with -O3 -flto on X86.

Diff Detail

Event Timeline

fhahn created this revision.Mar 5 2021, 10:04 AM
fhahn requested review of this revision.Mar 5 2021, 10:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 5 2021, 10:04 AM
lebedev.ri added inline comments.Mar 5 2021, 10:10 AM
llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
1316

I think you want to extend foldPHIArgOpIntoPHI().

nikic added a subscriber: nikic.Mar 5 2021, 10:12 AM

This needs tests for addrspacecast, I don't think it's handled correctly.

fhahn updated this revision to Diff 328982.Mar 8 2021, 5:29 AM

This needs tests for addrspacecast, I don't think it's handled correctly.

Right, stripPointerCasts also looks through addrspacecast. I updated the code to use CastInst::CreatePointerCast, which should automatically use the right type of cast.

fhahn added inline comments.Mar 8 2021, 5:35 AM
llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
1316

I'm not sure. The checks for foldPHIArgOpIntoPHI are more restrictive than what is needed for pointer casts I think (e.g. the incoming values need to be instructions with matching opcodes and the first one must only have a single user).

I don't think we need to be that restrictive for most pointer casts. For bit casts/geps, which should effectively be no-ops in the end, it should always be safe to replace a PHI with a bit cast. For addrspacecast, that may not be true and replacing a phi might add an extra 'real' instruction (if the incoming values are used elsewhere). I'd prefer to not handle addrspacecast here if we can get away with less restrictions in the bit cast case.

This indeed won't really work with foldPHIArgOpIntoPHI(), first because we actually don't create a new PHI,
secondly, this transform does not use-count restrictions, while foldPHIArgOpIntoPHI() has some.

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
1322–1328

I would recommend s/Inc/IV/

1325–1327

I would recommend a less brute-force approach.
Add a map<IV, stripped>, and don't redo the work if the result is known.

1325–1327

Well, i guess map is an overkill, just set<IV> should be plenty enough.

fhahn updated this revision to Diff 329037.Mar 8 2021, 8:45 AM

Adjust names, use set to avoid re-doing work.

fhahn added inline comments.Mar 8 2021, 8:46 AM
llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
1322–1328

Updated all the names

1325–1327

Done, although I am not sure how often this will trigger. It will at least for the initial step. Not sure if you had something different in mind?

lebedev.ri accepted this revision.Mar 8 2021, 8:56 AM

LGTM, thank you.

llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
1325–1327

This seems fine, thanks.
This is mostly to avoid worst-case scenarios with switches.

1326–1327

I'm being super nitpicky, but i'd suggest s/KnownEqual/CheckedIVs/

This revision is now accepted and ready to land.Mar 8 2021, 8:56 AM
This revision was automatically updated to reflect the committed changes.
fhahn added inline comments.Mar 9 2021, 3:41 AM
llvm/lib/Transforms/InstCombine/InstCombinePHI.cpp
1326–1327

Done