[LoadStoreVectorizer] Consider if operation is faster than before

[rampitec]

Summary

Compare a relative speed of misaligned accesses before and
after vectorization, not just check the new instruction is
not going to be slower.

Since no target now returns anything but 0 or 1 for Fast
argument of the allowsMisalignedMemoryAccesses this is still NFCI.

The subsequent patch will tune actual vaues of Fast on AMDGPU.

[foad]

llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp

1089

It is not at all clear to me whether it makes sense to compare FastBefore with Fast directly, or whether one of them needs to be multiplied by ChainSize. Especially since the definition of Fast is target-specific.

[rampitec]

llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp

1089

I thought about this, but 'fast' is not an actual memory transfer speed like Gb/s, it is a speed rank. Defining real memory speed will be hard to do, especially given alignment considerations. I do not think this is additive like a cost. It can be compared, but hardly added or multiplied. More so when an ultimate 'slow' is still zero.

[rampitec]

Rebased.

[arsenm]

llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp

1084–1085

Maybe rename to RelativeCost or something?

[rampitec]

Renamed 'Fast' used in the file to 'RelativeSpeed'.

[foad]

llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp

1089

I still don't understand how generic code can meaningfully compare, for example, one load with speed rank 42 vs two loads with speed rank 99. Are you supposed to ignore the number of loads completely, and just compare the speed rank numbers?

[rampitec]

llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp

1089

Yes, the rank behaves more like a throughput. It is not precisely a throughput because nobody can guarantee it, but a similar concept.

[rampitec]

ping

[rampitec]

ping

[arsenm]

Needs testcase

[rampitec]

In D124218#3932136, @arsenm wrote:

Needs testcase

This is NFCI. Even the next in stack (D124219) does not really change the codegen, just makes model more sane.

[rampitec]

In D124218#3934383, @rampitec wrote:

In D124218#3932136, @arsenm wrote:

Needs testcase

This is NFCI. Even the next in stack (D124219) does not really change the codegen, just makes model more sane.

This is really NFCI, since no targets use it and return a relative speed yet. However, subsequent D124219 could have some tests for sub-dword LDS operations with alignment smaller than 4. I will add these to the D124219.
The other patch which should depend on this (incomplete, as this one hung for a long time) is the scratch handling, it is always faster to use a dword access no matter what.

[arsenm]

llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp

1343–1344

Single quote newline

llvm/lib/Transforms/Vectorize/LoadStoreVectorizer.cpp

vectorizeLoadChain(ArrayRef<Instruction *> Chain,
                      SmallPtrSet<Instruction *, 16> *InstructionsProcessed);
 
   /// Vectorizes the store instructions in Chain.
   bool
   vectorizeStoreChain(ArrayRef<Instruction *> Chain,
                       SmallPtrSet<Instruction *, 16> *InstructionsProcessed);
 
   /// Check if this load/store access is misaligned accesses.
+  /// Returns a \p RelativeSpeed of an operation if allowed suitable to
+  /// compare to another result for the same \p AddressSpace and potentially
+  /// different \p Alignment and \p SzInBytes.
   bool accessIsMisaligned(unsigned SzInBytes, unsigned AddressSpace,
-                          Align Alignment);
+                          Align Alignment, unsigned &RelativeSpeed);
 };
 
 class LoadStoreVectorizerLegacyPass : public FunctionPass {
 public:
   static char ID;
 
   LoadStoreVectorizerLegacyPass() : FunctionPass(ID) {
     initializeLoadStoreVectorizerLegacyPassPass(*PassRegistry::getPassRegistry());
for (Instruction *I : Chain)
       dbgs() << "  " << *I << "\n";
   });
 
   // We won't try again to vectorize the elements of the chain, regardless of
   // whether we succeed below.
   InstructionsProcessed->insert(Chain.begin(), Chain.end());
 
   // If the store is going to be misaligned, don't vectorize it.
-  if (accessIsMisaligned(SzInBytes, AS, Alignment)) {
+  unsigned RelativeSpeed;
+  if (accessIsMisaligned(SzInBytes, AS, Alignment, RelativeSpeed)) {
     if (S0->getPointerAddressSpace() != DL.getAllocaAddrSpace()) {
+      unsigned SpeedBefore;
+      accessIsMisaligned(EltSzInBytes, AS, Alignment, SpeedBefore);
+      if (SpeedBefore > RelativeSpeed)
+        return false;
+
       auto Chains = splitOddVectorElts(Chain, Sz);
       bool Vectorized = false;
       Vectorized |= vectorizeStoreChain(Chains.first, InstructionsProcessed);
       Vectorized |= vectorizeStoreChain(Chains.second, InstructionsProcessed);
       return Vectorized;
     }
 
     Align NewAlign = getOrEnforceKnownAlignment(S0->getPointerOperand(),
if (ChainSize > VF || (VF != TargetVF && TargetVF < ChainSize)) {
     return Vectorized;
   }
 
   // We won't try again to vectorize the elements of the chain, regardless of
   // whether we succeed below.
   InstructionsProcessed->insert(Chain.begin(), Chain.end());
 
   // If the load is going to be misaligned, don't vectorize it.
-  if (accessIsMisaligned(SzInBytes, AS, Alignment)) {
+  unsigned RelativeSpeed;
+  if (accessIsMisaligned(SzInBytes, AS, Alignment, RelativeSpeed)) {
     if (L0->getPointerAddressSpace() != DL.getAllocaAddrSpace()) {
+      unsigned SpeedBefore;
+      accessIsMisaligned(EltSzInBytes, AS, Alignment, SpeedBefore);
+      if (SpeedBefore > RelativeSpeed)
+        return false;
+
       auto Chains = splitOddVectorElts(Chain, Sz);
       bool Vectorized = false;
       Vectorized |= vectorizeLoadChain(Chains.first, InstructionsProcessed);
       Vectorized |= vectorizeLoadChain(Chains.second, InstructionsProcessed);
       return Vectorized;
     }
 
     Align NewAlign = getOrEnforceKnownAlignment(L0->getPointerOperand(),
bool Vectorizer::vectorizeLoadChain(
   eraseInstructions(Chain);
 
   ++NumVectorInstructions;
   NumScalarsVectorized += Chain.size();
   return true;
 }
 
 bool Vectorizer::accessIsMisaligned(unsigned SzInBytes, unsigned AddressSpace,
-                                    Align Alignment) {
+                                    Align Alignment, unsigned &RelativeSpeed) {
+  RelativeSpeed = 0;
   if (Alignment.value() % SzInBytes == 0)
     return false;
 
-  unsigned Fast = 0;
   bool Allows = TTI.allowsMisalignedMemoryAccesses(F.getParent()->getContext(),
                                                    SzInBytes * 8, AddressSpace,
-                                                   Alignment, &Fast);
+                                                   Alignment, &RelativeSpeed);
   LLVM_DEBUG(dbgs() << "LSV: Target said misaligned is allowed? " << Allows
-                    << " and fast? " << Fast << "\n";);
-  return !Allows || !Fast;
+                    << " with relative speed = " << RelativeSpeed << '\n';);
+  return !Allows || !RelativeSpeed;
 }