Page MenuHomePhabricator

[SLP] Distinguish "demanded and shrinkable" from "demanded and not shrinkable" values when determining the minimum bitwidth
ClosedPublic

Authored by haicheng on Mar 24 2018, 9:53 AM.

Details

Summary

We use two approaches for determining the minimum bitwidth.

  1. Demanded bits
  2. Value tracking

If demanded bits doesn't result in a narrower type, we then try value tracking. We need this if we want to root SLP trees with the indices of getelementptr instructions since all the bits of the indices are demanded.

But there is a missing piece though. We need to be able to distinguish "demanded and shrinkable" from "demanded and not shrinkable". For example, the bits of %i in

%i = sext i32 %e1 to i64 
%gep = getelementptr inbounds i64, i64* %p, i64 %i

are demanded, but we can shrink %i's type to i32 because it won't change the result of the getelementptr. On the other hand, in

%tmp15 = sext i32 %tmp14 to i64 
%tmp16 = insertvalue { i64, i64 } undef, i64 %tmp15, 0

it doesn't make sense to shrink %tmp15 and we can skip the value tracking.

This patch still adds z|sext to demote list, but does not use value tracking if they are not shrinkable. So, cast<vect>, trunc <vect>, exctract <vect>, cast <extract> are only generated when they are useful (shrinkable). For now, this patch only considers gep index as a shrinkable usage.

Most words above are from @mssimpso .

Diff Detail

Repository
rL LLVM

Event Timeline

haicheng created this revision.Mar 24 2018, 9:53 AM
ABataev added inline comments.Mar 26 2018, 7:11 AM
lib/Transforms/Vectorize/SLPVectorizer.cpp
4336 ↗(On Diff #139716)

You don't capture anything in lambda, so remove & from the capture list.

4337 ↗(On Diff #139716)

What if only begin use is GEP and others are not?

test/Transforms/SLPVectorizer/AArch64/ext-trunc.ll
1 ↗(On Diff #139716)
  1. Commit this test separately as NFC with checks for the current version of the compiler.
  2. Generate test checks using utils/update_test_checks.py
3–4 ↗(On Diff #139716)

Pass this data as test arguments.

haicheng updated this revision to Diff 139800.Mar 26 2018, 9:09 AM
haicheng marked 3 inline comments as done.

Update to address Alexey's comments. Thank you very much.

lib/Transforms/Vectorize/SLPVectorizer.cpp
4337 ↗(On Diff #139716)

I think we've checked earlier that there is only one use here.

test/Transforms/SLPVectorizer/AArch64/ext-trunc.ll
1 ↗(On Diff #139716)

I will commit the separately.

lebedev.ri added inline comments.
test/Transforms/SLPVectorizer/AArch64/ext-trunc.ll
1 ↗(On Diff #139716)

> Commit this test separately as NFC with checks for the current version of the compiler.

I will commit the separately.

What i think was meant is, commit the test now, and rebase the differential ontop of that commit,
so the diff actually shows what this differential changes in the testcase.

haicheng added inline comments.Mar 26 2018, 9:22 AM
test/Transforms/SLPVectorizer/AArch64/ext-trunc.ll
1 ↗(On Diff #139716)

Thank you for pointing this out. I am working on it now.

ABataev added inline comments.Mar 26 2018, 9:22 AM
lib/Transforms/Vectorize/SLPVectorizer.cpp
4337 ↗(On Diff #139800)

You can use just R->user_back()

4337 ↗(On Diff #139716)

Add the assert, please

test/Transforms/SLPVectorizer/AArch64/ext-trunc.ll
1 ↗(On Diff #139716)

That's right

haicheng updated this revision to Diff 139811.Mar 26 2018, 10:10 AM

Rebase and update the patch.

haicheng marked 4 inline comments as done.Mar 26 2018, 10:14 AM
ABataev added inline comments.Mar 26 2018, 10:21 AM
lib/Transforms/Vectorize/SLPVectorizer.cpp
4338 ↗(On Diff #139811)

Yuu don't need to dereference here if you're using user_back()

test/Transforms/SLPVectorizer/AArch64/ext-trunc.ll
12–14 ↗(On Diff #139811)

Why you removed some checks generated by utils/update_test_checks.py?

haicheng updated this revision to Diff 139829.Mar 26 2018, 11:37 AM
haicheng marked 2 inline comments as done.

Updated to address Alexey's latest comments. Thank you again.

ABataev added inline comments.Mar 26 2018, 11:41 AM
test/Transforms/SLPVectorizer/AArch64/ext-trunc.ll
12–14 ↗(On Diff #139829)

Add the full check to the original test too

haicheng updated this revision to Diff 139833.Mar 26 2018, 12:17 PM
haicheng marked an inline comment as done.

Rebase the test case.

Kindly Ping

Hi Haicheng,

This looks fine to me, but please wait for Alexey to approve. Thanks.

This revision is now accepted and ready to land.Apr 2 2018, 8:15 AM
This revision was automatically updated to reflect the committed changes.