AMDGPU/GlobalISel: Fix combine crash because LI is not set in prelegalizer

[Petar.Avramovic]

Summary

Caused by legacy min/max combines (select + cmp) asking for legalizer info in prelegalizer (D135047 added combine to all_combines).
Combine still does not work for AMDGPU since destination opcode is custom, not legal. Similar combine works on DAG since it asks for legal or custom.

[Petar.Avramovic]

To my understanding prelegalizer now also requires legalizer info?

llvm/test/CodeGen/AMDGPU/cttz_zero_undef.ll

743 ↗

(On Diff #472703)

Change is caused by zextload being combined pre-legalizer

[arsenm]

llvm/test/CodeGen/AMDGPU/cttz_zero_undef.ll

1619 ↗

(On Diff #472703)

This is a regression without getting rid of one of these ands

[aemerson]

In D137274#3903010, @Petar.Avramovic wrote:

To my understanding prelegalizer now also requires legalizer info?

Yes, that's what the IsPreLegalize argument is for since we need LI to be valid.

[Petar.Avramovic]

Correction:

Change is caused by zextload being combined pre-legalizer

Change is caused by zextload NOT being combined pre-legalizer

s16 zextload is not legal and with LI availabe in prelegalizer combiner does not perform zextload combine.
Regression is caused by additional zext.

Using if(!isPreLegalize()) as an old equivalent of if(LI) to avoid regression but I am not sure if it is correct behavior with the prelegalizer now also checking for legality. Overall I would prefer to use isPreLegalize() in this patch and fix LI uses in prelegalizer in another patch if needed.

If we don't make zextload in prelegalizer (and let legalizer deal with it) it becomes very difficult to combine, we are left with zext + lowered any_extending_load. It would require known bits analysis that would try to treat that anyextending load as zero or sign extending load (whichever is better) and perform combine and change type of load. This is not visible in the asm since we select any_extending load in the same way as zero_extending load.

[arsenm]

Prelegalize combining to zextload pre-legalize even if illegal is useful

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp

for (auto &UseMI : MRI.use_nodbg_instructions(LoadReg)) {
     if (UseMI.getOpcode() == TargetOpcode::G_SEXT ||
         UseMI.getOpcode() == TargetOpcode::G_ZEXT ||
         (UseMI.getOpcode() == TargetOpcode::G_ANYEXT)) {
       const auto &MMO = LoadMI->getMMO();
       // For atomics, only form anyextending loads.
       if (MMO.isAtomic() && UseMI.getOpcode() != TargetOpcode::G_ANYEXT)
         continue;
       // Check for legality.
-      if (LI) {
+      if (!isPreLegalize()) {
         LegalityQuery::MemDesc MMDesc(MMO);
         unsigned CandidateLoadOpc = getExtLoadOpcForExtend(UseMI.getOpcode());
         LLT UseTy = MRI.getType(UseMI.getOperand(0).getReg());
         LLT SrcTy = MRI.getType(LoadMI->getPointerReg());
         if (LI->getAction({CandidateLoadOpc, {UseTy, SrcTy}, {MMDesc}})
                 .Action != LegalizeActions::Legal)
           continue;
       }
bool CombinerHelper::canCombineFMadOrFMA(MachineInstr &MI,
   const TargetOptions &Options = MF->getTarget().Options;
   LLT DstType = MRI.getType(MI.getOperand(0).getReg());
 
   if (CanReassociate &&
       !(Options.UnsafeFPMath || MI.getFlag(MachineInstr::MIFlag::FmReassoc)))
     return false;
 
   // Floating-point multiply-add with intermediate rounding.
-  HasFMAD = (LI && TLI.isFMADLegal(MI, DstType));
+  HasFMAD = (!isPreLegalize() && TLI.isFMADLegal(MI, DstType));
   // Floating-point multiply-add without intermediate rounding.
   bool HasFMA = TLI.isFMAFasterThanFMulAndFAdd(*MF, DstType) &&
                 isLegalOrBeforeLegalizer({TargetOpcode::G_FMA, {DstType}});
   // No valid opcode, do not combine.
   if (!HasFMAD && !HasFMA)
     return false;
 
   AllowFusionGlobally = Options.AllowFPOpFusion == FPOpFusion::Fast ||

llvm/lib/Target/AMDGPU/AMDGPUPreLegalizerCombiner.cpp

public:
 
   bool combine(GISelChangeObserver &Observer, MachineInstr &MI,
                MachineIRBuilder &B) const override;
 };
 
 bool AMDGPUPreLegalizerCombinerInfo::combine(GISelChangeObserver &Observer,
                                               MachineInstr &MI,
                                               MachineIRBuilder &B) const {
-  AMDGPUCombinerHelper Helper(Observer, B, /*IsPreLegalize*/ true, KB, MDT);
+  const auto *LI = MI.getMF()->getSubtarget().getLegalizerInfo();
+  AMDGPUCombinerHelper Helper(Observer, B, /*IsPreLegalize*/ true, KB, MDT, LI);
   AMDGPUPreLegalizerCombinerHelper PreLegalizerHelper(B, Helper);
   AMDGPUGenPreLegalizerCombinerHelper Generated(GeneratedRuleCfg, Helper,
                                                 PreLegalizerHelper);
 
   if (Generated.tryCombineAll(Observer, MI, B))
     return true;
 
   switch (MI.getOpcode()) {

llvm/test/CodeGen/AMDGPU/GlobalISel/select-to-fmin-fmax.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -global-isel -mtriple=amdgcn-amd-amdpal -mcpu=gfx900 -verify-machineinstrs %s -o - | FileCheck -check-prefixes=GCN %s
+
+define half @test_s16(half %a) #0 {
+; GCN-LABEL: test_s16:
+; GCN:       ; %bb.0: ; %entry
+; GCN-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
+; GCN-NEXT:    v_cmp_gt_f16_e32 vcc, 0, v0
+; GCN-NEXT:    v_cndmask_b32_e64 v0, v0, 0, vcc
+; GCN-NEXT:    s_setpc_b64 s[30:31]
+entry:
+  %fcmp = fcmp olt half %a, 0.0
+  %sel = select i1 %fcmp, half 0.0, half %a
+  ret half %sel
+}
+
+define float @test_s32(float %a) #0 {
+; GCN-LABEL: test_s32:
+; GCN:       ; %bb.0: ; %entry
+; GCN-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
+; GCN-NEXT:    v_cmp_gt_f32_e32 vcc, 0, v0
+; GCN-NEXT:    v_cndmask_b32_e64 v0, v0, 0, vcc
+; GCN-NEXT:    s_setpc_b64 s[30:31]
+entry:
+  %fcmp = fcmp olt float %a, 0.0
+  %sel = select i1 %fcmp, float 0.0, float %a
+  ret float %sel
+}
+
+define double @test_s64(double %a) #0 {
+; GCN-LABEL: test_s64:
+; GCN:       ; %bb.0: ; %entry
+; GCN-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
+; GCN-NEXT:    v_cmp_gt_f64_e32 vcc, 0, v[0:1]
+; GCN-NEXT:    v_cndmask_b32_e64 v0, v0, 0, vcc
+; GCN-NEXT:    v_cndmask_b32_e64 v1, v1, 0, vcc
+; GCN-NEXT:    s_setpc_b64 s[30:31]
+entry:
+  %fcmp = fcmp olt double %a, 0.0
+  %sel = select i1 %fcmp, double 0.0, double %a
+  ret double %sel
+}
+
+define <4 x half> @test_v4s16(<4 x half> %a) #0 {
+; GCN-LABEL: test_v4s16:
+; GCN:       ; %bb.0: ; %entry
+; GCN-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
+; GCN-NEXT:    s_mov_b32 s6, 0
+; GCN-NEXT:    v_lshrrev_b32_e32 v2, 16, v0
+; GCN-NEXT:    v_cmp_gt_f16_e32 vcc, 0, v0
+; GCN-NEXT:    v_cmp_lt_f16_sdwa s[4:5], v0, s6 src0_sel:WORD_1 src1_sel:DWORD
+; GCN-NEXT:    v_lshrrev_b32_e32 v3, 16, v1
+; GCN-NEXT:    v_cndmask_b32_e64 v4, v0, 0, vcc
+; GCN-NEXT:    v_cndmask_b32_e64 v0, v2, 0, s[4:5]
+; GCN-NEXT:    v_cmp_gt_f16_e32 vcc, 0, v1
+; GCN-NEXT:    v_cmp_lt_f16_sdwa s[4:5], v1, s6 src0_sel:WORD_1 src1_sel:DWORD
+; GCN-NEXT:    v_cndmask_b32_e64 v2, v1, 0, vcc
+; GCN-NEXT:    v_cndmask_b32_e64 v1, v3, 0, s[4:5]
+; GCN-NEXT:    s_mov_b32 s4, 0x5040100
+; GCN-NEXT:    v_perm_b32 v0, v0, v4, s4
+; GCN-NEXT:    v_perm_b32 v1, v1, v2, s4
+; GCN-NEXT:    s_setpc_b64 s[30:31]
+entry:
+  %fcmp = fcmp olt <4 x half> %a, zeroinitializer
+  %sel = select <4 x i1> %fcmp, <4 x half> zeroinitializer, <4 x half> %a
+  ret <4 x half> %sel
+}
+
+define <8 x half> @test_v8s16(<8 x half> %a) #0 {
+; GCN-LABEL: test_v8s16:
+; GCN:       ; %bb.0: ; %entry
+; GCN-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
+; GCN-NEXT:    s_mov_b32 s6, 0
+; GCN-NEXT:    v_lshrrev_b32_e32 v4, 16, v0
+; GCN-NEXT:    v_cmp_gt_f16_e32 vcc, 0, v0
+; GCN-NEXT:    v_cmp_lt_f16_sdwa s[4:5], v0, s6 src0_sel:WORD_1 src1_sel:DWORD
+; GCN-NEXT:    v_lshrrev_b32_e32 v5, 16, v1
+; GCN-NEXT:    v_cndmask_b32_e64 v8, v0, 0, vcc
+; GCN-NEXT:    v_cndmask_b32_e64 v0, v4, 0, s[4:5]
+; GCN-NEXT:    v_cmp_gt_f16_e32 vcc, 0, v1
+; GCN-NEXT:    v_cmp_lt_f16_sdwa s[4:5], v1, s6 src0_sel:WORD_1 src1_sel:DWORD
+; GCN-NEXT:    v_lshrrev_b32_e32 v6, 16, v2
+; GCN-NEXT:    v_cndmask_b32_e64 v4, v1, 0, vcc
+; GCN-NEXT:    v_cndmask_b32_e64 v1, v5, 0, s[4:5]
+; GCN-NEXT:    v_cmp_gt_f16_e32 vcc, 0, v2
+; GCN-NEXT:    v_cmp_lt_f16_sdwa s[4:5], v2, s6 src0_sel:WORD_1 src1_sel:DWORD
+; GCN-NEXT:    v_lshrrev_b32_e32 v7, 16, v3
+; GCN-NEXT:    v_cndmask_b32_e64 v5, v2, 0, vcc
+; GCN-NEXT:    v_cndmask_b32_e64 v2, v6, 0, s[4:5]
+; GCN-NEXT:    v_cmp_gt_f16_e32 vcc, 0, v3
+; GCN-NEXT:    v_cmp_lt_f16_sdwa s[4:5], v3, s6 src0_sel:WORD_1 src1_sel:DWORD
+; GCN-NEXT:    v_cndmask_b32_e64 v6, v3, 0, vcc
+; GCN-NEXT:    v_cndmask_b32_e64 v3, v7, 0, s[4:5]
+; GCN-NEXT:    s_mov_b32 s4, 0x5040100
+; GCN-NEXT:    v_perm_b32 v0, v0, v8, s4
+; GCN-NEXT:    v_perm_b32 v1, v1, v4, s4
+; GCN-NEXT:    v_perm_b32 v2, v2, v5, s4
+; GCN-NEXT:    v_perm_b32 v3, v3, v6, s4
+; GCN-NEXT:    s_setpc_b64 s[30:31]
+entry:
+  %fcmp = fcmp olt <8 x half> %a, zeroinitializer
+  %sel = select <8 x i1> %fcmp, <8 x half> zeroinitializer, <8 x half> %a
+  ret <8 x half> %sel
+}
+
+define <2 x float> @test_v2s32(<2 x float> %a) #0 {
+; GCN-LABEL: test_v2s32:
+; GCN:       ; %bb.0: ; %entry
+; GCN-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
+; GCN-NEXT:    v_cmp_gt_f32_e32 vcc, 0, v0
+; GCN-NEXT:    v_cndmask_b32_e64 v0, v0, 0, vcc
+; GCN-NEXT:    v_cmp_gt_f32_e32 vcc, 0, v1
+; GCN-NEXT:    v_cndmask_b32_e64 v1, v1, 0, vcc
+; GCN-NEXT:    s_setpc_b64 s[30:31]
+entry:
+  %fcmp = fcmp olt <2 x float> %a, zeroinitializer
+  %sel = select <2 x i1> %fcmp, <2 x float> zeroinitializer, <2 x float> %a
+  ret <2 x float> %sel
+}
+
+define <4 x float> @test_v4s32(<4 x float> %a) #0 {
+; GCN-LABEL: test_v4s32:
+; GCN:       ; %bb.0: ; %entry
+; GCN-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
+; GCN-NEXT:    v_cmp_gt_f32_e32 vcc, 0, v0
+; GCN-NEXT:    v_cndmask_b32_e64 v0, v0, 0, vcc
+; GCN-NEXT:    v_cmp_gt_f32_e32 vcc, 0, v1
+; GCN-NEXT:    v_cndmask_b32_e64 v1, v1, 0, vcc
+; GCN-NEXT:    v_cmp_gt_f32_e32 vcc, 0, v2
+; GCN-NEXT:    v_cndmask_b32_e64 v2, v2, 0, vcc
+; GCN-NEXT:    v_cmp_gt_f32_e32 vcc, 0, v3
+; GCN-NEXT:    v_cndmask_b32_e64 v3, v3, 0, vcc
+; GCN-NEXT:    s_setpc_b64 s[30:31]
+entry:
+  %fcmp = fcmp olt <4 x float> %a, zeroinitializer
+  %sel = select <4 x i1> %fcmp, <4 x float> zeroinitializer, <4 x float> %a
+  ret <4 x float> %sel
+}
+
+define <2 x double> @test_v2s64(<2 x double> %a) #0 {
+; GCN-LABEL: test_v2s64:
+; GCN:       ; %bb.0: ; %entry
+; GCN-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
+; GCN-NEXT:    v_cmp_gt_f64_e32 vcc, 0, v[0:1]
+; GCN-NEXT:    v_cmp_gt_f64_e64 s[4:5], 0, v[2:3]
+; GCN-NEXT:    v_cndmask_b32_e64 v0, v0, 0, vcc
+; GCN-NEXT:    v_cndmask_b32_e64 v2, v2, 0, s[4:5]
+; GCN-NEXT:    v_cndmask_b32_e64 v1, v1, 0, vcc
+; GCN-NEXT:    v_cndmask_b32_e64 v3, v3, 0, s[4:5]
+; GCN-NEXT:    s_setpc_b64 s[30:31]
+entry:
+  %fcmp = fcmp olt <2 x double> %a, zeroinitializer
+  %sel = select <2 x i1> %fcmp, <2 x double> zeroinitializer, <2 x double> %a
+  ret <2 x double> %sel
+}
+