As discussed in D11760, this patch moves the (V)PSRA(WD) arithmetic shift-by-constant folding to InstCombine to match the logical shift implementations.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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) |
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. |
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. |
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