Page MenuHomePhabricator

[CodeGen] Change getAnyExtOrTrunc to use SIGN_EXTEND for some constants
Needs ReviewPublic

Authored by david-arm on Mon, Nov 22, 3:57 AM.

Details

Summary

When we know the value we're extending is a constant that has the sign
bit set then it makes sense to use SIGN_EXTEND because this may improve
code quality in some cases, particularly when doing a constant splat of
an unpacked vector type. For example, for SVE when splatting the value
-1 into all elements of a vector of type <vscale x 2 x i32> the element
type will get promoted from i32 -> i64. In this case we want the splat
value to sign-extend from (i32 -1) -> (i64 -1), whereas currently it
zero-extends from (i32 -1) -> (i64 0xFFFFFFFF). Sign-extending the
constant means we can use a single mov immediate instruction.

New tests added here:

CodeGen/AArch64/sve-vector-splat.ll

I believe we see some code quality improvements in these existing
tests too:

CodeGen/PowerPC/combine_ext_trunc.ll
CodeGen/X86/vector-rotate-512.ll

Diff Detail

Event Timeline

david-arm created this revision.Mon, Nov 22, 3:57 AM
david-arm requested review of this revision.Mon, Nov 22, 3:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Nov 22, 3:57 AM

Couple of discussion points. I can see the rationale, but I wonder about some of the test changes and whether this could be revealing a latent bug.

llvm/test/CodeGen/X86/vector-fshl-512.ll
1107

Can anyone comment if the sign extensions in these constants are NFC?

llvm/test/CodeGen/X86/vector-shift-shl-512.ll
334

Can anyone comment if these sll -> mul changes are expected & harmless?

lebedev.ri added inline comments.
llvm/test/CodeGen/X86/vector-shift-shl-512.ll
334

These look like regressions to me.

david-arm added inline comments.Wed, Nov 24, 2:22 AM
llvm/test/CodeGen/X86/vector-shift-shl-512.ll
334

Hi @lebedev.ri, thanks for providing input here. Do you know if this means that there is a latent bug in the code where we should be explicitly using zero-extend here? I'm worried about cases where code is relying upon getAnyExtOrTrunc zero-extending.

RKSimon added inline comments.Wed, Nov 24, 5:35 AM
llvm/test/CodeGen/X86/vector-shift-shl-512.ll
334

I'm not certain, but vXi8 shl by constant will fold to vXi8 multiplies by (pow2) constant, and will then be extended to vXi16 to make use of PMULLW - the upper bits aren't demanded so were any-extended, which was treated as a zero-extension during constant folding which preserved the pow2 nature which allowed it to be lowered back to a vXi16 shl. My guess is that now that they are sign-extended, the pow2 isn't seen any more.

craig.topper added inline comments.
llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp
1315

There's no guarantee that the caller would use getAnyExtOrTrunc. I think this should be in the constant folding for getNode(ISD::ANY_EXTEND)

craig.topper added inline comments.Wed, Nov 24, 8:51 AM
llvm/test/CodeGen/AArch64/sve-vector-splat.ll
119

Pre-commit the new tests so we can see the change?