[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:358
 
+static void findDemandedElts(InstCombiner &IC, Value *UseV,
+                             Instruction *I, APInt &DemandedElts) {
----------------
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.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp:1106-1109
+    if (!II.getArgOperand(0)->getType()->isVectorTy())
+      break;
+
+    auto *IIVTy = cast<FixedVectorType>(II.getArgOperand(0)->getType());
----------------
It would be simpler to use dyn_cast and break if it fails.


================
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))
----------------
`dyn_cast` should be `isa` since you don't use the result. But what is this actually testing for? Would it be better to test hasNamedOperand?


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

https://reviews.llvm.org/D146737