Page MenuHomePhabricator

[LV] Preserve inbounds on created GEPs
ClosedPublic

Authored by dneilson on Apr 27 2018, 7:57 AM.

Details

Summary

This is a fix for PR23997.

The loop vectorizer is not preserving the inbounds property of GEPs that it creates.
This is inhibiting some optimizations. This patch preserves the inbounds property in
the case where a load/store is being fed by an inbounds GEP.

Diff Detail

Repository
rL LLVM

Event Timeline

dneilson created this revision.Apr 27 2018, 7:57 AM
hsaito added a subscriber: hsaito.Apr 27 2018, 9:47 AM
hsaito added inline comments.
lib/Transforms/Vectorize/LoopVectorize.cpp
3128 ↗(On Diff #144345)

Would you try using getLoadStorePointerOperand() from Instructions.h (see r327173) and hoist this code above "if (SI)", to cover both LI and SI at the same time?

3160 ↗(On Diff #144345)

I wonder why this isn't done using a single GEP...... If we use a single GEP, we can clean up a little more.

3197 ↗(On Diff #144345)

Address computation part of LI and SI code paths should be identical. We probably should clean this up, but I suppose I shouldn't be asking you to do that as part of this patch. Just a note.

dneilson added inline comments.Apr 27 2018, 12:29 PM
lib/Transforms/Vectorize/LoopVectorize.cpp
3128 ↗(On Diff #144345)

Didn't know that exists. Definitely will use that; makes this cleaner. Thanks for pointing it out.

3160 ↗(On Diff #144345)

Good question.

3197 ↗(On Diff #144345)

The clear copy/paste really struck me as well. I'll do an NFC to clean this up...

hsaito added inline comments.Apr 27 2018, 12:58 PM
lib/Transforms/Vectorize/LoopVectorize.cpp
3128 ↗(On Diff #144345)

Welcome. I wrote it. There used to be six getPointerOperand() file static functions. I didn't want to write a seventh one. Now it's readily available, it's time to advertise.

3138 ↗(On Diff #144345)

While you are at this, please look into whether isInbounds() is supported for vector gep. Whether anyone uses that is a different question, but losing information unnecessarily is sad. Same for Load side.

3197 ↗(On Diff #144345)

appreciated.

dneilson added inline comments.Apr 27 2018, 1:46 PM
lib/Transforms/Vectorize/LoopVectorize.cpp
3138 ↗(On Diff #144345)

Unless I've overlooked something, getOrCreateVectorValue() doesn't create GEPs (contrary to the name of the var it's being saved into). Looks like it does some sort of cache lookups, and if those fail then it creates actual vectors (splat or series of insertelements).

The only other GEP creation I see is in vectorizeInterleaveGroup() -- which looks like it could have the same issue as this PR.

hsaito added inline comments.Apr 27 2018, 1:56 PM
lib/Transforms/Vectorize/LoopVectorize.cpp
3138 ↗(On Diff #144345)

Looks like vector GEP already had InBounds. That answers my question.

// Create the new GEP. Note that this GEP may be a scalar if VF == 1,    
// but it should be a vector, otherwise.                                 
auto *NewGEP = GEP->isInBounds() 
                   ? Builder.CreateInBoundsGEP(Ptr, Indices) 
                   : Builder.CreateGEP(Ptr, Indices);
dneilson updated this revision to Diff 144559.Apr 30 2018, 7:57 AM
  • Rebaseline.
  • Also preserve inbounds when vectorizing interleaved groups.
hsaito added inline comments.Apr 30 2018, 10:07 AM
lib/Transforms/Vectorize/LoopVectorize.cpp
2421 ↗(On Diff #144559)

I thought I commented about this before, but maybe I forgot to press "Save" or "Submit".
This GEP is dead if Reverse is true. Would you change the code so that dead instruction won't be built?
Plan if-then-else structure should do.

hsaito added inline comments.Apr 30 2018, 10:17 AM
lib/Transforms/Vectorize/LoopVectorize.cpp
2421 ↗(On Diff #144559)

I meant "plain if-then-else". Sorry for the spam.

dneilson updated this revision to Diff 144587.Apr 30 2018, 10:30 AM
  • Address latest review comment.

LGTM. Thanks.

lib/Transforms/Vectorize/LoopVectorize.cpp
2427 ↗(On Diff #144587)

Changing this to single GEP will enable further clean up, but I think I already asked a lot of clean up and you delivered a lot. If this bothers me, I'll do it myself.

Let me try again after adding myself as a reviewer. LGTM.

hsaito accepted this revision.Apr 30 2018, 5:10 PM
This revision is now accepted and ready to land.Apr 30 2018, 5:10 PM
This revision was automatically updated to reflect the committed changes.

I believe this broke at least one test:

http://lab.llvm.org:8011/builders/clang-cmake-armv7-quick/builds/1233/steps/ninja%20check%201/logs/FAIL%3A%20LLVM%3A%3Apr23997.ll

******************** TEST 'LLVM :: Transforms/LoopVectorize/pr23997.ll' FAILED ********************
Script:
--
/home/buildslave/buildslave/clang-cmake-armv7-quick/stage1/bin/opt -S -loop-vectorize -dce -instcombine < /home/buildslave/buildslave/clang-cmake-armv7-quick/llvm/test/Transforms/LoopVectorize/pr23997.ll | /home/buildslave/buildslave/clang-cmake-armv7-quick/stage1/bin/FileCheck /home/buildslave/buildslave/clang-cmake-armv7-quick/llvm/test/Transforms/LoopVectorize/pr23997.ll
--
Exit Code: 1

Command Output (stderr):
--
/home/buildslave/buildslave/clang-cmake-armv7-quick/llvm/test/Transforms/LoopVectorize/pr23997.ll:17:15: error: expected string not found in input
; CHECK-NEXT: [[TMP3:%.*]] = icmp ugt i64 [[TMP2:%.*]], 1
              ^
<stdin>:16:2: note: scanning from here
 br label %loop
 ^
<stdin>:25:2: note: possible intended match here
 %.21 = icmp ult i64 %indvars.iv.next4, %2
 ^

--

********************

Can you take a look, please?

Ah, the test was added in this diff, but it doesn't (always?) pass.

Yes, this was the cause of the break. Put an X86 test in the general-arch directory.

Fixed by: https://reviews.llvm.org/rL331281