Page MenuHomePhabricator

[AMDGPU][GlobalISel] Avoid selecting S_PACK with constants
ClosedPublic

Authored by mbrkusanin on Nov 27 2020, 3:09 AM.

Details

Summary

If constants are hidden behind G_ANYEXT we can treat them same way as G_SEXT.
For that purpose we extend getConstantVRegValWithLookThrough with option
to handle G_ANYEXT same way as G_SEXT.

Diff Detail

Event Timeline

mbrkusanin created this revision.Nov 27 2020, 3:09 AM
mbrkusanin requested review of this revision.Nov 27 2020, 3:09 AM
foad added inline comments.Nov 27 2020, 3:31 AM
llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
980–988 ↗(On Diff #308003)

I would prefer not to add more ad hoc constant folding to this pass. Can you work out where the un-folded pack instructions are coming from in the first place, and fix it there instead?

mbrkusanin retitled this revision from [AMDGPU] Constant fold S_PACK instructions to [AMDGPU][GlobalISel] Avoid selecting S_PACK with constants.
mbrkusanin edited the summary of this revision. (Show Details)

Specific case from one of the tests:

%5:sgpr(s16) = G_FCONSTANT half 0xH4000
%7:sgpr(s32) = G_ANYEXT %5:sgpr(s16)
%4:sreg_32(<2 x s16>) = G_BUILD_VECTOR_TRUNC %7:sgpr(s32), %7:sgpr(s32)

will now be:

%4:sreg_32 = S_MOV_B32 1073758208

instead of:

%5:sreg_32 = S_MOV_B32 16384
%4:sreg_32 = S_PACK_LL_B32_B16 %5:sreg_32, %5:sreg_32
mbrkusanin edited the summary of this revision. (Show Details)Nov 27 2020, 5:51 AM
foad added a subscriber: qcolombet.Nov 27 2020, 5:55 AM
foad added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/Utils.h
155

I'm not sure if we need a flag for this. Could we do it unconditionally? @qcolombet ?

mbrkusanin added inline comments.Nov 27 2020, 5:58 AM
llvm/include/llvm/CodeGen/GlobalISel/Utils.h
155

There were no regressions for check-all when I tried it that way, but I wasn't sure if I should leave it like that.

foad added inline comments.Nov 27 2020, 6:01 AM
llvm/include/llvm/CodeGen/GlobalISel/Utils.h
155

I think it's probably safe to treat anyext the same as zext in this function, but maybe wait for comments from other globalisel maintainers (who may be on thanksgiving vacation).

foad added a comment.Nov 27 2020, 6:38 AM

Specific case from one of the tests:

%5:sgpr(s16) = G_FCONSTANT half 0xH4000
%7:sgpr(s32) = G_ANYEXT %5:sgpr(s16)
%4:sreg_32(<2 x s16>) = G_BUILD_VECTOR_TRUNC %7:sgpr(s32), %7:sgpr(s32)

Perhaps there should be a combine that simplifies this to G_BUILD_VECTOR %5, %5.

Specific case from one of the tests:

%5:sgpr(s16) = G_FCONSTANT half 0xH4000
%7:sgpr(s32) = G_ANYEXT %5:sgpr(s16)
%4:sreg_32(<2 x s16>) = G_BUILD_VECTOR_TRUNC %7:sgpr(s32), %7:sgpr(s32)

Perhaps there should be a combine that simplifies this to G_BUILD_VECTOR %5, %5.

This is specifically avoiding 16-bit inputs, we expanded the other direction. It could be a bitcast of a scalar constant. I'm trying a somewhat different strategy for 16-bit vectors than the DAG but not sure what the end state is going to look like

arsenm added inline comments.Nov 30 2020, 9:40 AM
llvm/lib/CodeGen/GlobalISel/Utils.cpp
313–316

I considered doing this in D84165 but then ran into some case that made me hesitate but I don't remember the details

I think this looks good, and we could even get rid of the AnyExtAsZext flag and do it unconditionally. Adding some globalisel folks to see if there are any objections.

paquette added inline comments.Jan 5 2021, 10:04 AM
llvm/include/llvm/CodeGen/GlobalISel/Utils.h
155

Maybe this should be LookThroughAnyExt? I imagine there are situations where we might want to treat G_ANYEXT as a sign extend.

e.g.

%cst:_(s32) = G_CONSTANT i32 -1
%cst_wide:_(s64) = G_ANYEXT %cst
...
%y:_(s64) = ...
%x:_(s64) = G_ADD %y, %cst_wide
foad added inline comments.Jan 5 2021, 10:30 AM
llvm/include/llvm/CodeGen/GlobalISel/Utils.h
155

You mean, change the name of the argument to LookThroughAnyExt? But what would it do? Currently this function has to either sign- or zero-extend whatever constant it finds, so that it can return a value of the right bit width.

mbrkusanin edited the summary of this revision. (Show Details)

Rename + G_ANYEXT will now be treated same as G_SEXT instead of G_ZEXT.

foad accepted this revision.Jan 15 2021, 9:23 AM

LGTM.

This revision is now accepted and ready to land.Jan 15 2021, 9:23 AM
dsanders added inline comments.Jan 20 2021, 9:52 AM
llvm/include/llvm/CodeGen/GlobalISel/Utils.h
155

Currently this function has to either sign- or zero-extend whatever constant it finds

I think Jessica was pointing out that which extend (if any) is the correct thing to do is context and target dependent.

To add to the target dependent side of that: G_ANYEXT is an extension with undefined bits and you're allowed to pick zero/sign-extend if that's convenient for your target but different targets will prefer sign or zero extend and some won't care either way (e.g. because they can operate on the smaller bitwidth). In the case of Mips, it's potentially width dependent due to some quirks of forward compatibility on that target where registers are theoretically sign extended to infinite width even if the value is supposed to be zero extended.