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

[Christopher Tetreault via Phabricator via llvm-commits]

ctetreau added a comment.

This all seams fairly reasonable to me, just some minor nitpicks. I'm not confident in giving this the final approval, but hopefully this can serve as a ping to those who are.



================
Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:1367
+
+    bool doNothing() const {
+      return (EVLParamStrategy == Legal) && (OpStrategy == Legal);
----------------
BIKESHED: "doNothing" sounds like a nop, not a test


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:137
+  ///    Number of vector elements.
+  Value *createStepVector(IRBuilder<> &Builder, Type *LaneTy, int32_t NumElems);
+
----------------
This could conceivably work for scalable vectors after https://lists.llvm.org/pipermail/llvm-dev/2021-January/147943.html is merged


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:225-228
+  auto *FirstOp = VPI.getOperand(0);
+  auto *SndOp = VPI.getOperand(1);
+
+  auto *Mask = VPI.getMaskParam();
----------------
please name the type.


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:248
+
+  auto *NewBinOp = Builder.CreateBinOp(OC, FirstOp, SndOp, VPI.getName());
+
----------------
please name the type


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:254
+
+void CachingVPExpander::discardEVLParameter(VPIntrinsic &VPI) {
+  LLVM_DEBUG(dbgs() << "Discard EVL parameter in " << VPI << "\n");
----------------
please name types where the typename does not appear on the RHS.


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:302-303
+  ElementCount ElemCount = VPI.getStaticVectorLength();
+  auto *VLMask = convertEVLToMask(Builder, OldEVLParam, ElemCount);
+  auto *NewMaskParam = Builder.CreateAnd(VLMask, OldMaskParam);
+  VPI.setMaskParam(NewMaskParam);
----------------
please name the type


================
Comment at: llvm/lib/CodeGen/ExpandVectorPredication.cpp:322-323
+  unsigned OC = VPI.getFunctionalOpcode();
+#define FIRST_BINARY_INST(X) unsigned FirstBinOp = X;
+#define LAST_BINARY_INST(X) unsigned LastBinOp = X;
+#include "llvm/IR/Instruction.def"
----------------
should these get undefined after the include?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78203