[LV] Add TTI::shouldMaximizeVectorBandwidth to allow enabling it per target

[kparzysz]

Summary

The default implementation returns false and keeps the current behavior.

This change is related to D44523 and D44574.

[hsaito]

I think this is much cleaner than the other way. Your target can return extremely high cost for super-inefficient "less than full vector" loads/stores, right?

Added the author of D33341.

[kparzysz]

Yes, we do mark those loads as expensive. However, it looks like the condition on line 6158 could potentially eliminate the desired VF if the estimated register pressure is too high.

[hsaito]

In D44735#1044569, @kparzysz wrote:

Yes, we do mark those loads as expensive. However, it looks like the condition on line 6158 could potentially eliminate the desired VF if the estimated register pressure is too high.

Between line 6162 and 6263, you can add something like

auto MinVF = TTI.getMinimumVF(SmallestType);
if (MinVF > MaxVF)
   MaxVF = MinVF;

Assuming that your load/store HW can be fooled between different data types of the same size (and I think most if not all are),
we really don't need to send in the actual data type. Just need the element data size. That'll minimize the change w/o losing generality.
I think the rest of the job can be (and should be) handled by your Target CG.

I suggest positioning D44735 as a simple refinement of D33341 and then add the above mentioned MinVF adjustment on top, as a separate patch.
Does that work?

[kparzysz]

In D44735#1044638, @hsaito wrote:

I suggest positioning D44735 as a simple refinement of D33341 and then add the above mentioned MinVF adjustment on top, as a separate patch.
Does that work?

This plan makes sense to me, but I'm not sure what you mean by the above. Do you want me to reopen D33341? The only change there was to enable the flag by default and update failing testcases, I'd like to keep it disabled by default.

[hsaito]

In D44735#1044658, @kparzysz wrote:

In D44735#1044638, @hsaito wrote:

I suggest positioning D44735 as a simple refinement of D33341 and then add the above mentioned MinVF adjustment on top, as a separate patch.
Does that work?

This plan makes sense to me, but I'm not sure what you mean by the above. Do you want me to reopen D33341? The only change there was to enable the flag by default and update failing testcases, I'd like to keep it disabled by default.

By refinement, I meant making it TTI based, keep it disabled by default, and enable it only for your target. Hopefully, that's easy enough for the rest of the community to quickly review/accept and has much lower chances to get reverted.
That alone I think addresses your immediate concerns, and then MinVF adjustment will add additional peace of mind for those targets that need such enforcement.

[kparzysz]

Ok, that's basically this patch as-is. I thought you wanted me to change something.

[craig.topper]

include/llvm/Analysis/TargetTransformInfo.h

1344 ↗

(On Diff #139286)

Is const needed here? Looking at the nearby methods, it seems only getRegisterBitWidth has it.

[kparzysz]

include/llvm/Analysis/TargetTransformInfo.h

1344 ↗

(On Diff #139286)

No, it's not needed. Should I remove it? What is the best practice here?

[craig.topper]

include/llvm/Analysis/TargetTransformInfo.h

1344 ↗

(On Diff #139286)

Looks like a lot of the methods in here were added by Chandler in one commit without the const. It looks like quite a few at end were added later with const. I didn't scroll down enough before to see them before. const seems more correct. So I guess leave it.

[kparzysz]

Ping.

[craig.topper]

LGTM

llvm/trunk/include/llvm/Analysis/TargetTransformInfo.h

public:
   unsigned getNumberOfRegisters(bool Vector) const;
 
   /// \return The width of the largest scalar or vector register type.
   unsigned getRegisterBitWidth(bool Vector) const;
 
   /// \return The width of the smallest vector register type.
   unsigned getMinVectorRegisterBitWidth() const;
 
+  /// \return True if the vectorization factor should be chosen to
+  /// make the vector of the smallest element type match the size of a
+  /// vector register. For wider element types, this could result in
+  /// creating vectors that span multiple vector registers.
+  /// If false, the vectorization factor will be chosen based on the
+  /// size of the widest element type.
+  bool shouldMaximizeVectorBandwidth(bool OptSize) const;
+
   /// \return True if it should be considered for address type promotion.
   /// \p AllowPromotionWithoutCommonHeader Set true if promoting \p I is
   /// profitable without finding other extensions fed by the same input.
   bool shouldConsiderAddressTypePromotion(
       const Instruction &I, bool &AllowPromotionWithoutCommonHeader) const;
 
   /// \return The size of a cache line in bytes.
   unsigned getCacheLineSize() const;
public:
   virtual int getIntImmCost(const APInt &Imm, Type *Ty) = 0;
   virtual int getIntImmCost(unsigned Opc, unsigned Idx, const APInt &Imm,
                             Type *Ty) = 0;
   virtual int getIntImmCost(Intrinsic::ID IID, unsigned Idx, const APInt &Imm,
                             Type *Ty) = 0;
   virtual unsigned getNumberOfRegisters(bool Vector) = 0;
   virtual unsigned getRegisterBitWidth(bool Vector) const = 0;
   virtual unsigned getMinVectorRegisterBitWidth() = 0;
+  virtual bool shouldMaximizeVectorBandwidth(bool OptSize) const = 0;
   virtual bool shouldConsiderAddressTypePromotion(
       const Instruction &I, bool &AllowPromotionWithoutCommonHeader) = 0;
   virtual unsigned getCacheLineSize() = 0;
   virtual llvm::Optional<unsigned> getCacheSize(CacheLevel Level) = 0;
   virtual llvm::Optional<unsigned> getCacheAssociativity(CacheLevel Level) = 0;
   virtual unsigned getPrefetchDistance() = 0;
   virtual unsigned getMinPrefetchStride() = 0;
   virtual unsigned getMaxPrefetchIterationsAhead() = 0;
unsigned getNumberOfRegisters(bool Vector) override {
     return Impl.getNumberOfRegisters(Vector);
   }
   unsigned getRegisterBitWidth(bool Vector) const override {
     return Impl.getRegisterBitWidth(Vector);
   }
   unsigned getMinVectorRegisterBitWidth() override {
     return Impl.getMinVectorRegisterBitWidth();
   }
+  bool shouldMaximizeVectorBandwidth(bool OptSize) const override {
+    return Impl.shouldMaximizeVectorBandwidth(OptSize);
+  }
   bool shouldConsiderAddressTypePromotion(
       const Instruction &I, bool &AllowPromotionWithoutCommonHeader) override {
     return Impl.shouldConsiderAddressTypePromotion(
         I, AllowPromotionWithoutCommonHeader);
   }
   unsigned getCacheLineSize() override {
     return Impl.getCacheLineSize();
   }

llvm/trunk/include/llvm/Analysis/TargetTransformInfoImpl.h

public:
   }
 
   unsigned getNumberOfRegisters(bool Vector) { return 8; }
 
   unsigned getRegisterBitWidth(bool Vector) const { return 32; }
 
   unsigned getMinVectorRegisterBitWidth() { return 128; }
 
+  bool shouldMaximizeVectorBandwidth(bool OptSize) const { return false; }
+
   bool
   shouldConsiderAddressTypePromotion(const Instruction &I,
                                      bool &AllowPromotionWithoutCommonHeader) {
     AllowPromotionWithoutCommonHeader = false;
     return false;
   }
 
   unsigned getCacheLineSize() { return 0; }

llvm/trunk/lib/Analysis/TargetTransformInfo.cpp

 unsigned TargetTransformInfo::getRegisterBitWidth(bool Vector) const {
   return TTIImpl->getRegisterBitWidth(Vector);
 }
 
 unsigned TargetTransformInfo::getMinVectorRegisterBitWidth() const {
   return TTIImpl->getMinVectorRegisterBitWidth();
 }
 
+bool TargetTransformInfo::shouldMaximizeVectorBandwidth(bool OptSize) const {
+  return TTIImpl->shouldMaximizeVectorBandwidth(OptSize);
+}
+
 bool TargetTransformInfo::shouldConsiderAddressTypePromotion(
     const Instruction &I, bool &AllowPromotionWithoutCommonHeader) const {
   return TTIImpl->shouldConsiderAddressTypePromotion(
       I, AllowPromotionWithoutCommonHeader);
 }
 
 unsigned TargetTransformInfo::getCacheLineSize() const {
   return TTIImpl->getCacheLineSize();

llvm/trunk/lib/Transforms/Vectorize/LoopVectorize.cpp

if (MaxVectorSize == 0) {
     // choosing a higher viable VF as done in the loop below.
     DEBUG(dbgs() << "LV: Clamping the MaxVF to the constant trip count: "
                  << ConstTripCount << "\n");
     MaxVectorSize = ConstTripCount;
     return MaxVectorSize;
   }
 
   unsigned MaxVF = MaxVectorSize;
-  if (MaximizeBandwidth && !OptForSize) {
+  if (TTI.shouldMaximizeVectorBandwidth(OptForSize) ||
+      (MaximizeBandwidth && !OptForSize)) {
     // Collect all viable vectorization factors larger than the default MaxVF
     // (i.e. MaxVectorSize).
     SmallVector<unsigned, 8> VFs;
     unsigned NewMaxVectorSize = WidestRegister / SmallestType;
     for (unsigned VS = MaxVectorSize * 2; VS <= NewMaxVectorSize; VS *= 2)
       VFs.push_back(VS);
 
     // For each VF calculate its register usage.