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

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

In D132591#3760792 <https://reviews.llvm.org/D132591#3760792>, @david-arm wrote:

> I guess if you had concerns about enabling this by default for all targets you could limit this to a RISCV TTI hook initially?

I have no concerns with simply enabling this.  We have no evidence of problems, and we can revert if actual performance issues are revealed.

If you, or another reviewer, has concerns I'll simply drop the patch.  As I said up front, I'm not strongly motivated to push this.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4519
+  // Scalarization isn't legal for scalable vector types
+  InstructionCost ScalarCost = InstructionCost::getInvalid();
+  if (!VF.isScalable()) {
----------------
david-arm wrote:
> 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)?
Will change.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4551
+  SafeDivisorCost += TTI.getCmpSelInstrCost(
+    Instruction::Select, ToVectorTy(I->getType(), VF),
+    ToVectorTy(Type::getInt1Ty(I->getContext()), VF),
----------------
david-arm wrote:
> 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);
> ?
Was in the code being moved already, but sure, will change. 


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7154
+      const auto [ScalarCost, SafeDivisorCost] = getDivRemSpeculationCost(I, VF);
+      return isScalarWithPredication(I, VF) ? ScalarCost : SafeDivisorCost;
     }
----------------
david-arm wrote:
> 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.
You're misreading the code.  We need to return a cost regardless of the value of the cl::opt.  The only thing that changes is which one we return,


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132591