[InstCombine] Cleanup the TFE/LWE check in AMDGPU SimplifyDemanded

[nhaehnle]

Summary

The fix added in r352904 is not quite correct, or rather misleading:

1. When the texfailctrl (TFC) argument was non-constant, the fix assumed non-TFE/LWE, which is incorrect.

2. Regardless, this code path cannot even be hit for correct TFE/LWE-enabled calls, because those return a struct. Added a test case for those for completeness.

Change-Id: I92d314dbc67a2670f6d7adaab765ef45f56a49cf

[dstuttard]

LGTM

[arsenm]

Does it actually matter? I thought since this needs to be a constant, this just needs to not crash

test/Transforms/InstCombine/AMDGPU/amdgcn-demanded-vector-elts.ll

2420 ↗

(On Diff #185028)

There's an _ in the name?

[hliao]

LGTM

[nhaehnle]

In D57681#1383030, @arsenm wrote:

Does it actually matter? I thought since this needs to be a constant, this just needs to not crash

It's certainly a bit on the paranoid side. I was considering the possibility that somebody passes a value that only becomes constant after inlining.

test/Transforms/InstCombine/AMDGPU/amdgcn-demanded-vector-elts.ll

2420 ↗

(On Diff #185028)

You mean the sl_v4f32i32s part? That's what the intrinsic name got canonicalized to for some reason. I'm not really uptodate on the mangling of struct types...

[arsenm]

In D57681#1383911, @nhaehnle wrote:

In D57681#1383030, @arsenm wrote:

Does it actually matter? I thought since this needs to be a constant, this just needs to not crash

It's certainly a bit on the paranoid side. I was considering the possibility that somebody passes a value that only becomes constant after inlining.

I've been hoping to start working soon on a way to define which intrinsic operands are required to be constants to avoid hacks in global isel, so hopefully we won't need to worry about this much longer.

test/Transforms/InstCombine/AMDGPU/amdgcn-demanded-vector-elts.ll

2420 ↗

(On Diff #185028)

Yes, I wasn't sure how that works

llvm/trunk/lib/Transforms/InstCombine/InstCombineInternal.h

Value *simplifyShrShlDemandedBits(
       const APInt &ShlOp1, const APInt &DemandedMask, KnownBits &Known);
 
   /// Tries to simplify operands to an integer instruction based on its
   /// demanded bits.
   bool SimplifyDemandedInstructionBits(Instruction &Inst);
 
   Value *simplifyAMDGCNMemoryIntrinsicDemanded(IntrinsicInst *II,
                                                APInt DemandedElts,
-                                               int DmaskIdx = -1,
-                                               int TFCIdx = -1);
+                                               int DmaskIdx = -1);
 
   Value *SimplifyDemandedVectorElts(Value *V, APInt DemandedElts,
                                     APInt &UndefElts, unsigned Depth = 0);
 
   /// Canonicalize the position of binops relative to shufflevector.
   Instruction *foldVectorBinop(BinaryOperator &Inst);
 
   /// Given a binary operator, cast instruction, or select which has a PHI node

llvm/trunk/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp

if ((BitMask1 & DemandedMask) == (BitMask2 & DemandedMask)) {
 
     return InsertNewInstWith(New, *Shl);
   }
 
   return nullptr;
 }
 
 /// Implement SimplifyDemandedVectorElts for amdgcn buffer and image intrinsics.
+///
+/// Note: This only supports non-TFE/LWE image intrinsic calls; those have
+///       struct returns.
 Value *InstCombiner::simplifyAMDGCNMemoryIntrinsicDemanded(IntrinsicInst *II,
                                                            APInt DemandedElts,
-                                                           int DMaskIdx,
-                                                           int TFCIdx) {
+                                                           int DMaskIdx) {
   unsigned VWidth = II->getType()->getVectorNumElements();
   if (VWidth == 1)
     return nullptr;
 
-  // Need to change to new instruction format
-  bool TFELWEEnabled = false;
-  if (TFCIdx > 0) {
-    if (ConstantInt *TFC = dyn_cast<ConstantInt>(II->getArgOperand(TFCIdx)))
-      TFELWEEnabled =    TFC->getZExtValue() & 0x1  // TFE
-                      || TFC->getZExtValue() & 0x2; // LWE
-  }
-
-  if (TFELWEEnabled)
-    return nullptr; // TFE not yet supported
-
   ConstantInt *NewDMask = nullptr;
 
   if (DMaskIdx < 0) {
     // Pretend that a prefix of elements is demanded to simplify the code
     // below.
     DemandedElts = (1 << DemandedElts.getActiveBits()) - 1;
   } else {
     ConstantInt *DMask = dyn_cast<ConstantInt>(II->getArgOperand(DMaskIdx));
case Instruction::Call: {
     case Intrinsic::amdgcn_buffer_load:
     case Intrinsic::amdgcn_buffer_load_format:
     case Intrinsic::amdgcn_raw_buffer_load:
     case Intrinsic::amdgcn_raw_buffer_load_format:
     case Intrinsic::amdgcn_struct_buffer_load:
     case Intrinsic::amdgcn_struct_buffer_load_format:
       return simplifyAMDGCNMemoryIntrinsicDemanded(II, DemandedElts);
     default: {
-      if (getAMDGPUImageDMaskIntrinsic(II->getIntrinsicID()))
-        return simplifyAMDGCNMemoryIntrinsicDemanded(
-            II, DemandedElts, 0, II->getNumArgOperands() - 2);
+      if (getAMDGPUImageDMaskIntrinsic(II->getIntrinsicID())) {
+        LLVM_DEBUG(
+          Value *TFC = II->getArgOperand(II->getNumOperands() - 2);
+          assert(!isa<ConstantInt>(TFC) ||
+                 dyn_cast<ConstantInt>(TFC)->getZExtValue() == 0);
+        );
+
+        return simplifyAMDGCNMemoryIntrinsicDemanded(II, DemandedElts, 0);
+      }
 
       break;
     }
     } // switch on IntrinsicID
     break;
   } // case Call
   } // switch on Opcode
 

llvm/trunk/test/Transforms/InstCombine/AMDGPU/amdgcn-demanded-vector-elts.ll

 define protected <4 x half> @__llvm_amdgcn_image_sample_d_1darray_v4f16_f32_f32(i32, float, float, float, float, <8 x i32>, <4 x i32>, i1 zeroext, i32, i32) local_unnamed_addr {
   %tmp = tail call <4 x half> @llvm.amdgcn.image.sample.d.1darray.v4f16.f32.f32(i32 %0, float %1, float %2, float %3, float %4, <8 x i32> %5, <4 x i32> %6, i1 zeroext %7, i32 %8, i32 %9) #1
   ret <4 x half> %tmp
 }
 
 declare <4 x float> @llvm.amdgcn.image.getresinfo.1d.v4f32.i32(i32, i32, <8 x i32>, i32, i32) #1
 declare <4 x half> @llvm.amdgcn.image.sample.d.1darray.v4f16.f32.f32(i32, float, float, float, float, <8 x i32>, <4 x i32>, i1, i32, i32)
 
+; --------------------------------------------------------------------
+; TFE / LWE
+; --------------------------------------------------------------------
+
+; CHECK-LABEL: @extract_elt0_tfe_image_load_1d_v4f32i32_i32(
+; CHECK-NEXT: %data = call { <4 x float>, i32 } @llvm.amdgcn.image.load.1d.sl_v4f32i32s.i32(i32 15, i32 %s, <8 x i32> %rsrc, i32 0, i32 1)
+define amdgpu_ps float @extract_elt0_tfe_image_load_1d_v4f32i32_i32(i32 %s, <8 x i32> inreg %rsrc) #0 {
+  %data = call { <4 x float>, i32 } @llvm.amdgcn.image.load.1d.sl_v4f32i32s.i32(i32 15, i32 %s, <8 x i32> %rsrc, i32 0, i32 1)
+  %rgba = extractvalue { <4 x float>, i32 } %data, 0
+  %elt0 = extractelement <4 x float> %rgba, i32 0
+  ret float %elt0
+}
+
+declare {<4 x float>, i32} @llvm.amdgcn.image.load.1d.sl_v4f32i32s.i32(i32, i32, <8 x i32>, i32, i32) #1
+
 attributes #0 = { nounwind }
 attributes #1 = { nounwind readonly }
 
 !0 = !{float 2.500000e+00}