Page MenuHomePhabricator

[InstCombine] Properly change GEP type when reassociating loop invariant GEP chains
ClosedPublic

Authored by dneilson on Apr 4 2018, 1:48 PM.

Details

Summary

This is a fix to PR37005.

Essentially, rL328539 ([InstCombine] reassociate loop invariant GEP chains to enable LICM) contains a bug
whereby it will convert:
%src = getelementptr inbounds i8, i8* %base, <2 x i64> %val
%res = getelementptr inbounds i8, <2 x i8*> %src, i64 %val2
into:
%src = getelementptr inbounds i8, i8* %base, i64 %val2
%res = getelementptr inbounds i8, <2 x i8*> %src, <2 x i64> %val

By swapping the index operands if the GEPs are in a loop, and %val is loop variant while %val2
is loop invariant.

This fix recreates new GEP instructions if the index operand swap would result in the type
of %src changing from vector to scalar, or vice versa.

Diff Detail

Repository
rL LLVM

Event Timeline

dneilson created this revision.Apr 4 2018, 1:48 PM
dneilson added inline comments.Apr 5 2018, 6:19 AM
lib/Transforms/InstCombine/InstructionCombining.cpp
1710 ↗(On Diff #141047)

I need to change this. We should be putting the new src instruction at the same location as the one it's replacing. Otherwise, it might be possible to get into unknown weirdness.

dneilson updated this revision to Diff 141146.Apr 5 2018, 6:25 AM
  • Change insertion point of replacement src GEP instruction.
sebpop accepted this revision.Apr 5 2018, 9:43 AM

Looks good. Thanks!

lib/Transforms/InstCombine/InstructionCombining.cpp
1709 ↗(On Diff #141047)

Let's move SO0 before SO0Ty and use it in that computation.

This revision is now accepted and ready to land.Apr 5 2018, 9:43 AM
dneilson updated this revision to Diff 141191.Apr 5 2018, 11:53 AM
  • Make review copy match what will be landed.
This revision was automatically updated to reflect the committed changes.