[PATCH] D132591: [LV] Use safe-divisor lowering for fixed vectors if profitable

[David Sherwood via Phabricator via llvm-commits]

david-arm added a comment.

I like the idea of this patch - I imagine it's a bit similar to the setCostBasedWideningDecision function where we make a widening decision before doing the actual full loop costing. I guess if you had concerns about enabling this by default for all targets you could limit this to a RISCV TTI hook initially?



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4519
+  // Scalarization isn't legal for scalable vector types
+  InstructionCost ScalarCost = InstructionCost::getInvalid();
+  if (!VF.isScalable()) {
----------------
This is just a minor point, but I wonder if ScalarizationCost is a better name, to distinguish from an actual scalar cost (i.e. VF=1)?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4551
+  SafeDivisorCost += TTI.getCmpSelInstrCost(
+    Instruction::Select, ToVectorTy(I->getType(), VF),
+    ToVectorTy(Type::getInt1Ty(I->getContext()), VF),
----------------
In order to avoid creating the vector type twice here and below, perhaps create a local variable, i.e. 

  Type VecTy = ToVectorTy(I->getType(), VF);
?


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7154
+      const auto [ScalarCost, SafeDivisorCost] = getDivRemSpeculationCost(I, VF);
+      return isScalarWithPredication(I, VF) ? ScalarCost : SafeDivisorCost;
     }
----------------
It seems a bit unfortunate that we're calling getDivRemSpeculationCost twice here - once in this function and once in isScalarWithPredication. Would it be better to pull the switch statement out of isScalarWithPredication into a small, common function, i.e.:

  bool isDivRemScalarWithPredication(Instruction *I, ElementCount VF) {
    switch (ForceSafeDivisor) {
    case cl::BOU_UNSET: {
      const auto [ScalarCost, SafeDivisorCost] = getDivRemSpeculationCost(I, VF);
      return ScalarCost< SafeDivisorCost;
    }
    case cl::BOU_TRUE:
      return false;
    case cl::BOU_FALSE:
      return true;
    };
  }

Then isScalarWithPredication and this function can just call isDivRemScalarWithPredication. It also restricts the cost calculations to when ForceSafeDivisor is cl::BOU_UNSET.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132591