This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Move SSE2/AVX2 arithmetic vector shift folding to instcombiner
ClosedPublic

Authored by RKSimon on Aug 9 2015, 6:16 AM.

Details

Summary

As discussed in D11760, this patch moves the (V)PSRA(WD) arithmetic shift-by-constant folding to InstCombine to match the logical shift implementations.

Diff Detail

Repository
rL LLVM

Event Timeline

RKSimon updated this revision to Diff 31618.Aug 9 2015, 6:16 AM
RKSimon retitled this revision from to [InstCombine] Move SSE2/AVX2 arithmetic vector shift folding to instcombiner.
RKSimon updated this object.
RKSimon added reviewers: andreadb, qcolombet, mkuper.
RKSimon set the repository for this revision to rL LLVM.
RKSimon added a subscriber: llvm-commits.
mkuper edited edge metadata.Aug 9 2015, 6:42 AM

Thanks Simon,

I have a few minor questions about testing, other than that, looks good.

lib/Transforms/InstCombine/InstCombineCalls.cpp
261 ↗(On Diff #31618)

Do we have an ISel test that these AShrs get lowered correctly? If we don't, should we?

test/CodeGen/X86/combine-avx2-intrinsics.ll
6 ↗(On Diff #31618)

We still want to test these combines, right? (Only as part of InstCombine, not ISel)

RKSimon added inline comments.Aug 9 2015, 8:30 AM
lib/Transforms/InstCombine/InstCombineCalls.cpp
261 ↗(On Diff #31618)

We have the tests in test\CodeGen\X86\vector-shift-ashr-*.ll

test/CodeGen/X86/combine-avx2-intrinsics.ll
6 ↗(On Diff #31618)

I can add these shift accumulation tests as well if you wish but I will keep the simple tests in there too. The x86-vector-shifts.ll test file already has some general constant folding tests at the end that do various forms of accumulation.

mkuper added inline comments.Aug 9 2015, 8:32 AM
lib/Transforms/InstCombine/InstCombineCalls.cpp
261 ↗(On Diff #31618)

Ok.

test/CodeGen/X86/combine-avx2-intrinsics.ll
6 ↗(On Diff #31618)

I'm just against removing (working) regression tests on principle. :-)
But yes, I meant in addition, not instead of the simple test.

majnemer added inline comments.
lib/Transforms/InstCombine/InstCombineCalls.cpp
202 ↗(On Diff #31618)

Would it make sense to assert (LogicalShift || !ShiftLeft) seeing as how there is no arithmetic left shift. Alternatively, you could make that state impossible by construction by using an enum for the three states.

Thanks guys - I'll get an updated patch up as soon as I can.

lib/Transforms/InstCombine/InstCombineCalls.cpp
202 ↗(On Diff #31618)

No problem - I'll add the assert.

test/CodeGen/X86/combine-avx2-intrinsics.ll
6 ↗(On Diff #31618)

OK I'll transfer the tests over - note that I'll have to refactor them as they won't lower anymore.

andreadb edited edge metadata.Aug 10 2015, 2:12 AM

Hi Simon,

I saw that Michael and David already reviewed your patch.
If you address their comments then the patch looks good to me too. Thanks!

In future, we should also move the target specific combine rules on sse/avx blend intrinsic calls from 'PerformINTRINSIC_WO_CHAINCombine' to InstCombine.

Thanks,
-Andrea

RKSimon updated this revision to Diff 31666.Aug 10 2015, 7:04 AM
RKSimon edited edge metadata.

Updated based on feedback.

mkuper accepted this revision.Aug 10 2015, 7:08 AM
mkuper edited edge metadata.

LGTM

This revision is now accepted and ready to land.Aug 10 2015, 7:08 AM
This revision was automatically updated to reflect the committed changes.
llvm/trunk/test/CodeGen/X86/combine-avx2-intrinsics.ll