[PATCH] D78203: [VP,Integer,#2] ExpandVectorPredication pass

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:67
+
+static VPTransform parseOverrideOption(const std::string TextOpt) {
+  return StringSwitch<VPTransform>(TextOpt) VPINTERNAL_VPLEGAL_CASES;
----------------
Is this supposed to be a std::string& ?


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:75
+static bool anyExpandVPOverridesSet() {
+  return (EVLTransformOverride != "") || (MaskTransformOverride != "");
+}
----------------
Would !EVLTransformOverride.empty() || !MaskTransformOverride.empty() be better?


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:90
+    return false;
+  return ConstVec->isAllOnesValue();
+}
----------------
return ConstVec && ConstVec->isAllOnesValues();


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:95
+static Constant *getSafeDivisor(Type *DivTy) {
+  assert(DivTy->isIntOrIntVectorTy());
+  return ConstantInt::get(DivTy, 1u, false);
----------------
No message


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:165
+
+  // Determine how and whether the VPIntrinsic \p VPI shall be expanded.
+  // This overrides TTI with the cl::opts listed at the top of this file.
----------------
Is this supposed to be a doxygen comment? It only uses 2 slashes


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:174
+
+  // Expand VP ops in \p F according to \p TTI.
+  bool expandVectorPredication();
----------------
Does this comment go to the constructor? the \p references don't apply to this function. But it's not a doxygen comment.


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:185
+
+  for (int32_t Idx = 0; Idx < NumElems; ++Idx) {
+    ConstElems.push_back(ConstantInt::get(LaneTy, Idx, false));
----------------
Drop curly braces.


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:326
+
+  if (FirstBinOp <= OC && OC <= LastBinOp) {
+    return expandPredicationInBinaryOperator(Builder, VPI);
----------------
Can this use Instruction::isBinaryOp?


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:392
+    sanitizeStrategy(I, VPStrat);
+    if (!VPStrat.shouldDoNothing()) {
+      Worklist.emplace_back(VPI, VPStrat);
----------------
Drop curly braces


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:400
+  // Transform all VPIntrinsics on the worklist.
+  LLVM_DEBUG(dbgs() << "\n:::: Transforming instructions (" << Worklist.size()
+                    << ") ::::\n");
----------------
This might read better as

```
"\n::::Transforming " << Worklist.size() << " Instructions"
```

Then it's obvious the number in parens is a count.



================
Comment at: llvm/lib/IR/IntrinsicInst.cpp:266
+  auto MaskPos = GetMaskParamPos(getIntrinsicID());
+  assert(MaskPos.hasValue());
+  setArgOperand(MaskPos.getValue(), NewMask);
----------------
getValue() will assert itself won't it?


================
Comment at: llvm/lib/IR/IntrinsicInst.cpp:279
+  auto EVLPos = GetVectorLengthParamPos(getIntrinsicID());
+  assert(EVLPos.hasValue());
+  setArgOperand(EVLPos.getValue(), NewEVL);
----------------
Same


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78203