[AMDGPU][GlobalISel] Avoid selecting S_PACK with constants

[mbrkusanin]

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.

[foad]

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]

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

[foad]

llvm/include/llvm/CodeGen/GlobalISel/Utils.h

146

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

[mbrkusanin]

llvm/include/llvm/CodeGen/GlobalISel/Utils.h

146

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]

llvm/include/llvm/CodeGen/GlobalISel/Utils.h

146

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]

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.

[arsenm]

In D92219#2420126, @foad wrote:

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]

llvm/lib/CodeGen/GlobalISel/Utils.cpp

298–301

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

[foad]

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]

llvm/include/llvm/CodeGen/GlobalISel/Utils.h

146

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]

llvm/include/llvm/CodeGen/GlobalISel/Utils.h

146

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]

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

[foad]

LGTM.

[dsanders]

llvm/include/llvm/CodeGen/GlobalISel/Utils.h

146

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.

llvm/include/llvm/CodeGen/GlobalISel/Utils.h

 };
 /// If \p VReg is defined by a statically evaluable chain of
 /// instructions rooted on a G_F/CONSTANT (\p LookThroughInstrs == true)
 /// and that constant fits in int64_t, returns its value as well as the
 /// virtual register defined by this G_F/CONSTANT.
 /// When \p LookThroughInstrs == false this function behaves like
 /// getConstantVRegVal.
 /// When \p HandleFConstants == false the function bails on G_FCONSTANTs.
+/// When \p AnyExtAsZext == true the function treats G_ANYEXT same as G_ZEXT.
 Optional<ValueAndVReg>
 getConstantVRegValWithLookThrough(Register VReg, const MachineRegisterInfo &MRI,
                                   bool LookThroughInstrs = true,
-                                  bool HandleFConstants = true);
+                                  bool HandleFConstants = true,
+                                  bool AnyExtAsZext = false);
 const ConstantFP* getConstantFPVRegVal(Register VReg,
                                        const MachineRegisterInfo &MRI);
 
 /// See if Reg is defined by an single def instruction that is
 /// Opcode. Also try to do trivial folding if it's a COPY with
 /// same types. Returns null otherwise.
 MachineInstr *getOpcodeDef(unsigned Opcode, Register Reg,
                            const MachineRegisterInfo &MRI);

llvm/lib/CodeGen/GlobalISel/Utils.cpp

assert((!ValAndVReg || ValAndVReg->VReg == VReg) &&
          "Value found while looking through instrs");
   if (!ValAndVReg)
     return None;
   return ValAndVReg->Value;
 }
 
 Optional<ValueAndVReg> llvm::getConstantVRegValWithLookThrough(
     Register VReg, const MachineRegisterInfo &MRI, bool LookThroughInstrs,
-    bool HandleFConstant) {
+    bool HandleFConstant, bool AnyExtAsZext) {
   SmallVector<std::pair<unsigned, unsigned>, 4> SeenOpcodes;
   MachineInstr *MI;
   auto IsConstantOpcode = [HandleFConstant](unsigned Opcode) {
     return Opcode == TargetOpcode::G_CONSTANT ||
            (HandleFConstant && Opcode == TargetOpcode::G_FCONSTANT);
   };
   auto GetImmediateValue = [HandleFConstant,
                             &MRI](const MachineInstr &MI) -> Optional<APInt> {
if (!CstVal.isFPImm()) {
              "Value bitwidth doesn't match definition type");
       return Val;
     }
     return CstVal.getFPImm()->getValueAPF().bitcastToAPInt();
   };
   while ((MI = MRI.getVRegDef(VReg)) && !IsConstantOpcode(MI->getOpcode()) &&
          LookThroughInstrs) {
     switch (MI->getOpcode()) {
+    case TargetOpcode::G_ANYEXT:
+      if (!AnyExtAsZext)
+        return None;
+      LLVM_FALLTHROUGH;
     case TargetOpcode::G_TRUNC:
     case TargetOpcode::G_SEXT:
     case TargetOpcode::G_ZEXT:
       SeenOpcodes.push_back(std::make_pair(
           MI->getOpcode(),
           MRI.getType(MI->getOperand(0).getReg()).getSizeInBits()));
       VReg = MI->getOperand(1).getReg();
       break;
while (!SeenOpcodes.empty()) {
     std::pair<unsigned, unsigned> OpcodeAndSize = SeenOpcodes.pop_back_val();
     switch (OpcodeAndSize.first) {
     case TargetOpcode::G_TRUNC:
       Val = Val.trunc(OpcodeAndSize.second);
       break;
     case TargetOpcode::G_SEXT:
       Val = Val.sext(OpcodeAndSize.second);
       break;
+    case TargetOpcode::G_ANYEXT:
     case TargetOpcode::G_ZEXT:
       Val = Val.zext(OpcodeAndSize.second);
       break;
     }
   }
 
   if (Val.getBitWidth() > 64)
     return None;

llvm/lib/Target/AMDGPU/AMDGPUInstructionSelector.cpp

bool AMDGPUInstructionSelector::selectG_BUILD_VECTOR_TRUNC(
   Register Src0 = MI.getOperand(1).getReg();
   Register Src1 = MI.getOperand(2).getReg();
   if (MRI->getType(Src0) != S32)
     return false;
 
   const DebugLoc &DL = MI.getDebugLoc();
   MachineBasicBlock *BB = MI.getParent();
 
-  auto ConstSrc1 = getConstantVRegValWithLookThrough(Src1, *MRI, true, true);
+  auto ConstSrc1 =
+      getConstantVRegValWithLookThrough(Src1, *MRI, true, true, true);
   if (ConstSrc1) {
-    auto ConstSrc0 = getConstantVRegValWithLookThrough(Src0, *MRI, true, true);
+    auto ConstSrc0 =
+        getConstantVRegValWithLookThrough(Src0, *MRI, true, true, true);
     if (ConstSrc0) {
       uint32_t Lo16 = static_cast<uint32_t>(ConstSrc0->Value) & 0xffff;
       uint32_t Hi16 = static_cast<uint32_t>(ConstSrc1->Value) & 0xffff;
 
       BuildMI(*BB, &MI, DL, TII.get(AMDGPU::S_MOV_B32), Dst)
         .addImm(Lo16 | (Hi16 << 16));
       MI.eraseFromParent();
       return RBI.constrainGenericRegister(Dst, AMDGPU::SReg_32RegClass, *MRI);

llvm/test/CodeGen/AMDGPU/GlobalISel/inst-select-build-vector-trunc.v2s16.mir

 legalized:       true
 regBankSelected: true
 tracksRegLiveness: true
 
 body: |
   bb.0:
 
     ; GFX9-LABEL: name: test_build_vector_trunc_s_v2s16_anyext_constant_anyext_constant
-    ; GFX9: [[S_MOV_B32_:%[0-9]+]]:sreg_32 = S_MOV_B32 123
-    ; GFX9: [[S_MOV_B32_1:%[0-9]+]]:sreg_32 = S_MOV_B32 456
-    ; GFX9: [[S_PACK_LL_B32_B16_:%[0-9]+]]:sreg_32 = S_PACK_LL_B32_B16 [[S_MOV_B32_]], [[S_MOV_B32_1]]
-    ; GFX9: S_ENDPGM 0, implicit [[S_PACK_LL_B32_B16_]]
+    ; GFX9: [[S_MOV_B32_:%[0-9]+]]:sreg_32 = S_MOV_B32 29884539
+    ; GFX9: S_ENDPGM 0, implicit [[S_MOV_B32_]]
     %0:sgpr(s16) = G_CONSTANT i16 123
     %1:sgpr(s16) = G_CONSTANT i16 456
     %2:sgpr(s32) = G_ANYEXT %0
     %3:sgpr(s32) = G_ANYEXT %1
     %4:sgpr(<2 x s16>) = G_BUILD_VECTOR_TRUNC %2, %3
     S_ENDPGM 0, implicit %4
 ...
 

llvm/test/CodeGen/AMDGPU/GlobalISel/llvm.amdgcn.fdot2.ll

; GFX10-NEXT:    s_setpc_b64 s[30:31]
   %r = call float @llvm.amdgcn.fdot2(<2 x half> %a, <2 x half> %b, float %neg.c, i1 false)
   ret float %r
 }
 
 define float @v_fdot2_inline_literal_a(<2 x half> %b, float %c) {
 ; GFX906-LABEL: v_fdot2_inline_literal_a:
 ; GFX906:       ; %bb.0:
 ; GFX906-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
-; GFX906-NEXT:    s_movk_i32 s4, 0x4000
-; GFX906-NEXT:    s_pack_ll_b32_b16 s4, s4, s4
-; GFX906-NEXT:    v_dot2_f32_f16 v0, s4, v0, v1
+; GFX906-NEXT:    v_dot2_f32_f16 v0, 2.0, v0, v1 op_sel_hi:[0,1,1]
 ; GFX906-NEXT:    s_setpc_b64 s[30:31]
 ;
 ; GFX10-LABEL: v_fdot2_inline_literal_a:
 ; GFX10:       ; %bb.0:
 ; GFX10-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 ; GFX10-NEXT:    s_waitcnt_vscnt null, 0x0
-; GFX10-NEXT:    s_movk_i32 s4, 0x4000
+; GFX10-NEXT:    v_dot2_f32_f16 v0, 2.0, v0, v1 op_sel_hi:[0,1,1]
 ; GFX10-NEXT:    ; implicit-def: $vcc_hi
-; GFX10-NEXT:    s_pack_ll_b32_b16 s4, s4, s4
-; GFX10-NEXT:    v_dot2_f32_f16 v0, s4, v0, v1
 ; GFX10-NEXT:    s_setpc_b64 s[30:31]
   %ret = tail call float @llvm.amdgcn.fdot2(<2 x half> <half 2.0, half 2.0>, <2 x half> %b, float %c, i1 false)
   ret float %ret
 }
 
 define float @v_fdot2_inline_literal_b(<2 x half> %a, float %c) {
 ; GFX906-LABEL: v_fdot2_inline_literal_b:
 ; GFX906:       ; %bb.0:
 ; GFX906-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
-; GFX906-NEXT:    s_movk_i32 s4, 0x4000
-; GFX906-NEXT:    s_pack_ll_b32_b16 s4, s4, s4
-; GFX906-NEXT:    v_dot2_f32_f16 v0, v0, s4, v1
+; GFX906-NEXT:    v_dot2_f32_f16 v0, v0, 2.0, v1 op_sel_hi:[1,0,1]
 ; GFX906-NEXT:    s_setpc_b64 s[30:31]
 ;
 ; GFX10-LABEL: v_fdot2_inline_literal_b:
 ; GFX10:       ; %bb.0:
 ; GFX10-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 ; GFX10-NEXT:    s_waitcnt_vscnt null, 0x0
-; GFX10-NEXT:    s_movk_i32 s4, 0x4000
+; GFX10-NEXT:    v_dot2_f32_f16 v0, v0, 2.0, v1 op_sel_hi:[1,0,1]
 ; GFX10-NEXT:    ; implicit-def: $vcc_hi
-; GFX10-NEXT:    s_pack_ll_b32_b16 s4, s4, s4
-; GFX10-NEXT:    v_dot2_f32_f16 v0, v0, s4, v1
 ; GFX10-NEXT:    s_setpc_b64 s[30:31]
   %ret = tail call float @llvm.amdgcn.fdot2(<2 x half> %a, <2 x half> <half 2.0, half 2.0>, float %c, i1 false)
   ret float %ret
 }
 
 define float @v_fdot2_inline_literal_c(<2 x half> %a, <2 x half> %b) {
 ; GFX906-LABEL: v_fdot2_inline_literal_c:
 ; GFX906:       ; %bb.0: