[CostModel] Use MaybeAlign in getMemoryOpCost

[samparker]

Summary

Use MaybeAlign in the generic API, like we do in the backends and in BasicTTI.

[gchatelet]

Why is it superior to use MaybeAlign instead of Align?
MaybeAlign forces the user to consider the None case, this is more logic to consider.
I fail to see why loosening the interface is desirable. Did I miss something?

[samparker]

Well, firstly I'd just like the interface to be consistent throughout the cost model and, since the IR doesn't force memory operations to have an alignment, I don't see why we'd try to enforce that here.

[gchatelet]

In D80924#2068291, @samparker wrote:

Well, firstly I'd just like the interface to be consistent throughout the cost model and, since the IR doesn't force memory operations to have an alignment, I don't see why we'd try to enforce that here.

We're moving to a world where we want the alignment to be explicit at the IR level for loads and stores (see D77454 and D79900).
So for consistency - which I agree is important - we should go in the opposite direction and straighten the type system with Align wherever possible.
I'm not super familiar with the code TBH, are there any memory operations handled by getMemoryOpCost that are neither load nor store?

[samparker]

We're moving to a world where we want the alignment to be explicit at the IR level for loads and stores

Ah, thanks for the update, it sounds like a good direction.

are there any memory operations handled by getMemoryOpCost that are neither load nor store?

As far as I can tell, no... but I also see little enforcement. The masked intrinsics have their own API but I guess there's nothing stopping a backend from using this API to get a baseline for any of it's 'memory' operations. I will update this patch to move to Align instead.

[samparker]

Looking again at the backends, it looks like a very involved task to migrate to Align - particularly because getMaskedMemoryOpCost still uses an unsigned value. So I'd still like to get this patch in for consistency, without having to make large changes to several of the backends (of which I'm not even sure what I'd need to do). As it stands, I assume there's some implicit casting going so these base implementations aren't even receiving an Align object in most cases.

[gchatelet]

In D80924#2073067, @samparker wrote:

Looking again at the backends, it looks like a very involved task to migrate to Align - particularly because getMaskedMemoryOpCost still uses an unsigned value. So I'd still like to get this patch in for consistency, without having to make large changes to several of the backends (of which I'm not even sure what I'd need to do). As it stands, I assume there's some implicit casting going so these base implementations aren't even receiving an Align object in most cases.

Migrating to Align/MaybeAlign is indeed quite an involved task.
I've been working on it for almost a year now and that's why I opposed this patch ( to me it's like deconstructing what I've been fighting for :) )

Align/MaybeAlign are especially designed so there are no implicit casting. One have to call .value() and test for value availability in case of MaybeAlign.
I haven't had time to convert all the APIs yet but I intend to. In the meantime I'd appreciate if you could leave it as is.

If you feel strongly about it you can still submit this patch but this will be more work on my plate.

[samparker]

I appreciate that this has consumed a lot of your time :) The only reason why this is a problem for me is that these generic layers can't talk to each other at the moment, and I need this so that I can move some code around in D80984. I actually have a sneaking feeling it was me that introduced this API change in the first place and I just missed these ones.

[arsenm]

We should remove MaybeAlign entirely. Nothing is ever truly missing the alignment, and Align(1) is always a acceptable degenerate value

[samparker]

We should remove MaybeAlign entirely. Nothing is ever truly missing the alignment, and Align(1) is always a acceptable degenerate value.

Okay, so for my case where TTIImpl can't pass the object to BasicTTI, I could just check whether alignment is assigned, and if not, then just pass Align(1)?

[arsenm]

In D80924#2073798, @samparker wrote:

We should remove MaybeAlign entirely. Nothing is ever truly missing the alignment, and Align(1) is always a acceptable degenerate value.

Okay, so for my case where TTIImpl can't pass the object to BasicTTI, I could just check whether alignment is assigned, and if not, then just pass Align(1)?

In D80924#2073798, @samparker wrote:

We should remove MaybeAlign entirely. Nothing is ever truly missing the alignment, and Align(1) is always a acceptable degenerate value.

Okay, so for my case where TTIImpl can't pass the object to BasicTTI, I could just check whether alignment is assigned, and if not, then just pass Align(1)?

Yes. The broken API before was that you were supposed to check the ABI type alignment if the alignment wasn't set on the load/store instruction. Those have been fixed, and there should be a shrinking number of contexts where the alignment is "missing"

[gchatelet]

In D80924#2073798, @samparker wrote:

We should remove MaybeAlign entirely. Nothing is ever truly missing the alignment, and Align(1) is always a acceptable degenerate value.

Okay, so for my case where TTIImpl can't pass the object to BasicTTI, I could just check whether alignment is assigned, and if not, then just pass Align(1)?

Yes, thx @arsenm for pointing this out.

[samparker]

Thank you both.

llvm/include/llvm/Analysis/TargetTransformInfo.h

                          TTI::TargetCostKind CostKind = TTI::TCK_RecipThroughput,
                          const Instruction *I = nullptr) const;
 
   /// \return The expected cost of vector Insert and Extract.
   /// Use -1 to indicate that there is no information on the index value.
   int getVectorInstrCost(unsigned Opcode, Type *Val, unsigned Index = -1) const;
 
   /// \return The cost of Load and Store instructions.
-  int getMemoryOpCost(unsigned Opcode, Type *Src, Align Alignment,
+  int getMemoryOpCost(unsigned Opcode, Type *Src, MaybeAlign Alignment,
                       unsigned AddressSpace,
                       TTI::TargetCostKind CostKind = TTI::TCK_RecipThroughput,
                       const Instruction *I = nullptr) const;
 
   /// \return The cost of masked Load and Store instructions.
   int getMaskedMemoryOpCost(
     unsigned Opcode, Type *Src, unsigned Alignment, unsigned AddressSpace,
     TTI::TargetCostKind CostKind = TTI::TCK_RecipThroughput) const;

                                        VectorType *VecTy, unsigned Index) = 0;
   virtual int getCFInstrCost(unsigned Opcode,
                              TTI::TargetCostKind CostKind) = 0;
   virtual int getCmpSelInstrCost(unsigned Opcode, Type *ValTy, Type *CondTy,
                                  TTI::TargetCostKind CostKind,
                                  const Instruction *I) = 0;
   virtual int getVectorInstrCost(unsigned Opcode, Type *Val,
                                  unsigned Index) = 0;
-  virtual int getMemoryOpCost(unsigned Opcode, Type *Src, Align Alignment,
+  virtual int getMemoryOpCost(unsigned Opcode, Type *Src, MaybeAlign Alignment,
                               unsigned AddressSpace,
                               TTI::TargetCostKind CostKind,
                               const Instruction *I) = 0;
   virtual int getMaskedMemoryOpCost(unsigned Opcode, Type *Src,
                                     unsigned Alignment,
                                     unsigned AddressSpace,
                                     TTI::TargetCostKind CostKind) = 0;
   virtual int getGatherScatterOpCost(

   int getCmpSelInstrCost(unsigned Opcode, Type *ValTy, Type *CondTy,
                          TTI::TargetCostKind CostKind,
                          const Instruction *I) override {
     return Impl.getCmpSelInstrCost(Opcode, ValTy, CondTy, CostKind, I);
   }
   int getVectorInstrCost(unsigned Opcode, Type *Val, unsigned Index) override {
     return Impl.getVectorInstrCost(Opcode, Val, Index);
   }
-  int getMemoryOpCost(unsigned Opcode, Type *Src, Align Alignment,
+  int getMemoryOpCost(unsigned Opcode, Type *Src, MaybeAlign Alignment,
                       unsigned AddressSpace, TTI::TargetCostKind CostKind,
                       const Instruction *I) override {
     return Impl.getMemoryOpCost(Opcode, Src, Alignment, AddressSpace,
                                 CostKind, I);
   }
   int getMaskedMemoryOpCost(unsigned Opcode, Type *Src, unsigned Alignment,
                             unsigned AddressSpace,
                             TTI::TargetCostKind CostKind) override {

llvm/include/llvm/Analysis/TargetTransformInfoImpl.h

unsigned getCmpSelInstrCost(unsigned Opcode, Type *ValTy, Type *CondTy,
                               const Instruction *I) const {
     return 1;
   }
 
   unsigned getVectorInstrCost(unsigned Opcode, Type *Val, unsigned Index) {
     return 1;
   }
 
-  unsigned getMemoryOpCost(unsigned Opcode, Type *Src, Align Alignment,
+  unsigned getMemoryOpCost(unsigned Opcode, Type *Src, MaybeAlign Alignment,
                            unsigned AddressSpace, TTI::TargetCostKind CostKind,
                            const Instruction *I) const {
     return 1;
   }
 
   unsigned getMaskedMemoryOpCost(unsigned Opcode, Type *Src, unsigned Alignment,
                                  unsigned AddressSpace,
                                  TTI::TargetCostKind CostKind) {

llvm/lib/Analysis/TargetTransformInfo.cpp

 int TargetTransformInfo::getVectorInstrCost(unsigned Opcode, Type *Val,
                                             unsigned Index) const {
   int Cost = TTIImpl->getVectorInstrCost(Opcode, Val, Index);
   assert(Cost >= 0 && "TTI should not produce negative costs!");
   return Cost;
 }
 
 int TargetTransformInfo::getMemoryOpCost(unsigned Opcode, Type *Src,
-                                         Align Alignment, unsigned AddressSpace,
+                                         MaybeAlign Alignment,
+                                         unsigned AddressSpace,
                                          TTI::TargetCostKind CostKind,
                                          const Instruction *I) const {
   assert((I == nullptr || I->getOpcode() == Opcode) &&
          "Opcode should reflect passed instruction.");
   int Cost = TTIImpl->getMemoryOpCost(Opcode, Src, Alignment, AddressSpace,
                                       CostKind, I);
   assert(Cost >= 0 && "TTI should not produce negative costs!");
   return Cost;