[PATCH] D146737: [AMDGPU] Trim zero components from buffer and image stores

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp:1111
+    auto *IIVTy = dyn_cast<FixedVectorType>(II.getArgOperand(0)->getType());
+    if (!IIVTy)
+      break;
----------------
You don't use `IIVTy` for anything else so just test `isa<FixedVectorType>(...)` instead.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp:1115
+
+    int DMaskIdx = dyn_cast<ConstantInt>(II.getArgOperand(1)) ? 1 : -1;
+    if (simplifyAMDGCNMemoryIntrinsicDemanded(IC, II, DemandedElts, DMaskIdx, false))
----------------
matejam wrote:
> arsenm wrote:
> > foad wrote:
> > > I think these intrinsics all have `immarg` on the dmask argument, so it should always be a constant?
> > Yes, should be able to just cast<ConstantInt>
> Both image and buffer intrinsics have the same body in this switch.
> For buffer instructions the first operand is not a constant, that is why I used dyn_cast<ConstantInt>.
> I could use isa<ConstantInt>.
> I'm not sure how would I check for hasNamedOperand, because II is a call instruction.
You definitely should not rely on the second argument of the buffer intrinsics **not** being a constant! Instead you should test the opcode.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp:358
 
+static void findDemandedElts(InstCombiner &IC, Value *UseV,
+                             Instruction *I, APInt &DemandedElts) {
----------------
matejam wrote:
> foad wrote:
> > This function needs a better name now to explain what it does, and a comment, and should return APInt instead of taking an APInt& argument.
> Is the name trimTrailingZerosInVector more explanatory?
Sure.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146737/new/

https://reviews.llvm.org/D146737