[PATCH] D146737: [AMDGPU] Default component broadcast store

[Jay Foad via Phabricator via llvm-commits]

foad added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp:359-362
+  // ValueComponent contains all distinct components.
+  SmallVector<Value *, 4> ValueComponents(VWidth, nullptr);
+  // Components contains the positions of set components.
+  SmallVector<int, 4> Components(VWidth, -1);
----------------
Maybe call them `ComponentIndices` and `ComponentValues`?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp:365
+
+  while (!isa<UndefValue>(FirstArg)) {
+    CurInst = dyn_cast<Instruction>(FirstArg);
----------------
I wonder if the whole of this loop could be replaced by calling llvm::computeKnownBits with an appropriate DemandedElts mask to test each element from the last to the first.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp:1145
+
+    auto *IIVTy = cast<FixedVectorType>(II.getOperand(0)->getType());
+    unsigned VWidth = IIVTy->getNumElements();
----------------
Use getArgOperand throughout for consistency?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp:1148
+    APInt DemandedElts = APInt::getAllOnes(VWidth);
+    if (auto *SrcVec = dyn_cast<Instruction>(II.getArgOperand(0)))
+      DemandedElts = findDemandedElts(SrcVec);
----------------
Why does this need to be an Instruction? Couldn't it be a Constant? E.g. if it was an all zeroes constant, that should be simplified.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp:1180
+    // that means that the intrinsic has already been optimized.
+    if (DMaskVal > (1u << VWidth))
+      break;
----------------
Should be `>=`


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUInstCombineIntrinsic.cpp:1350-1351
+    if (NewNumElts == 1)
+      return IC.Builder.CreateInsertElement(UndefValue::get(IIVTy), NewCall,
+                                            DemandedElts.countr_zero());
+
----------------
Keep the braces around this, because it is more than one physical line.


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

https://reviews.llvm.org/D146737