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

[David Sherwood via Phabricator via llvm-commits]

david-arm added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:7154
+      const auto [ScalarCost, SafeDivisorCost] = getDivRemSpeculationCost(I, VF);
+      return isScalarWithPredication(I, VF) ? ScalarCost : SafeDivisorCost;
     }
----------------
reames wrote:
> 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,
My apologies, yes you're right. However, my previous comment still stands - we're calculating the costs twice, which feels inefficient. In that case, it's still possible to refactor this to get the costs just once. For example,

  case Instruction::UDiv:
  case Instruction::SDiv:
  case Instruction::URem:
  case Instruction::SRem:
    const auto [ScalarCost, SafeDivisorCost] = getDivRemSpeculationCost(I, VF);
    return isDivRemScalarWithPredication(ScalarCost, SafeDivisorCost) ? ScalarCost : SafeDivisorCost;

where

  bool isDivRemScalarWithPredication(InstructionCost ScalarCost, InstructionCost  SafeDivisorCost) {
    switch (ForceSafeDivisor) {
    case cl::BOU_UNSET:
      return ScalarCost< SafeDivisorCost;
    case cl::BOU_TRUE:
      return false;
    case cl::BOU_FALSE:
      return true;
    };
  }

and do something similar for the div/rem case in isScalarWithPredication.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D132591