[SVE][AArch64] Fix TypeSize warning in loop vectorization legality

[joechrisellis]

Summary

The warning would fire when calling isDereferenceableAndAlignedInLoop
with a scalable load. Calling isDereferenceableAndAlignedInLoop with a
scalable load would result in the use of the now deprecated implicit
cast of TypeSize to uint64_t through the overloaded operator.

This patch fixes this issue by:

• no longer considering vector loads as candidates in canVectorizeWithIfConvert. This doesn't make sense in the context of identifying scalar loads to vectorize.

• making use of getFixedSize inside isDereferenceableAndAlignedInLoop -- this removes the dependency on the deprecated interface, and will trigger an assertion error if the function is ever called with a scalable type.

[peterwaller-arm]

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp

1021

I'd be tempted to make this if (!foo()) continue, so that the "passing" case is in the body of the loop, thus making it straightforward to add or remove conditions from the loop.

[joechrisellis]

Address peterwaller-arm's comments regarding control flow readability

[sdesmalen]

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp

1021

nit: yet now the diff is bigger, it seems simpler to just add one extra line:

      if (LI && !mustSuppressSpeculation(*LI) &&
+         !LI->getType()->isVectorType() &&
          isDereferenceableAndAlignedInLoop(LI, TheLoop, SE, *DT))

llvm/test/Transforms/LoopVectorize/AArch64/sve-scalable-load-in-loop.ll

4

Can you add a description of what this test is supposed to be testing?

Given that there are no CHECK lines that verify the output of opt, you may as well leave the first call to FileCheck out.

[peterwaller-arm]

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp

1021

On this occasion he was following https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code

Is there an etiquette to minimize size-of-diff?

[peterwaller-arm]

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp

1021

I guess that would be covered by the introduction [0] to the guide. It explicitly says that large scale refactoring isn't wanted, but that if you're changing it anyway then update it. My thinking is that that this sort of small patch it would be legitimate to update the style in the surrounding to follow the guide with respect to the early exit recommendation.

So what do others think on balance: legitimate in this case or not?

[0] https://llvm.org/docs/CodingStandards.html#introduction

[david-arm]

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp

1021

I understand where you're coming from here @peterwaller-arm and there have been occasions in the past where reviewers have also asked me to redo code to make things a bit clearer and follow a preferred style. I don't think it's wrong to undertake small scale refactoring. I guess it's just in this particular case the early-exit link you posted above gives an example where the if block is very large, i.e. the docs say "This code has several problems if the body of the 'if' is large." In this particular patch the if block is a single line statement, so perhaps it's fine to have a multi-condition if statement?

[david-arm]

llvm/test/Transforms/LoopVectorize/AArch64/sve-scalable-load-in-loop.ll

4

There is a CHECK-LABEL line below. but I think if you keep the first FileCheck then it's good to add at least one more CHECK line, perhaps something like:

; CHECK-NOT: vector.body

? I've seen other tests do something similar.

[sdesmalen]

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp

1021

One of the main benefits of rewriting to a loop to one with early exits is when there are multiple nested conditions, e.g.

for(...) {
  if (auto *X = dyn_cast<something>(....)) 
    if (<some condition>)
      // ...
      if (<some other condition>)
        // ... hand the case
 }

Where it is better to pick those apart to avoid the multiple levels of indentation.

With the coding standard there is some freedom of interpretation, because it doesn't give any quantifications for what a 'large' body or condition is and leaves the trade-off between "size of patch" and "refactoring clarifies the code" up to the developer. That's intentional to avoid people rewriting lots of existing code into canonical form because they need to add one line. In this particular case, because the indentation is just one level and the body of the if-statement has only one statement, I think that balance tips in the favour of the one-line change over the refactoring.

[joechrisellis]

Revert to single-line if statement, add description to the test, and add an
additional check line.

[sdesmalen]

LGTM!

llvm/lib/Analysis/Loads.cpp

 
 bool llvm::isDereferenceableAndAlignedInLoop(LoadInst *LI, Loop *L,
                                              ScalarEvolution &SE,
                                              DominatorTree &DT) {
   auto &DL = LI->getModule()->getDataLayout();
   Value *Ptr = LI->getPointerOperand();
 
   APInt EltSize(DL.getIndexTypeSizeInBits(Ptr->getType()),
-                DL.getTypeStoreSize(LI->getType()));
+                DL.getTypeStoreSize(LI->getType()).getFixedSize());
   const Align Alignment = LI->getAlign();
 
   Instruction *HeaderFirstNonPHI = L->getHeader()->getFirstNonPHI();
 
   // If given a uniform (i.e. non-varying) address, see if we can prove the
   // access is safe within the loop w/o needing predication.
   if (L->isLoopInvariant(Ptr))
     return isDereferenceableAndAlignedPointer(Ptr, Alignment, EltSize, DL,

llvm/lib/Transforms/Vectorize/LoopVectorizationLegality.cpp

for (BasicBlock *BB : TheLoop->blocks()) {
     // For a block which requires predication, a address may be safe to access
     // in the loop w/o predication if we can prove dereferenceability facts
     // sufficient to ensure it'll never fault within the loop. For the moment,
     // we restrict this to loads; stores are more complicated due to
     // concurrency restrictions.
     ScalarEvolution &SE = *PSE.getSE();
     for (Instruction &I : *BB) {
       LoadInst *LI = dyn_cast<LoadInst>(&I);
-      if (LI && !mustSuppressSpeculation(*LI) &&
+      if (LI && !LI->getType()->isVectorTy() && !mustSuppressSpeculation(*LI) &&
           isDereferenceableAndAlignedInLoop(LI, TheLoop, SE, *DT))
         SafePointers.insert(LI->getPointerOperand());
     }
   }
 
   // Collect the blocks that need predication.
   BasicBlock *Header = TheLoop->getHeader();
   for (BasicBlock *BB : TheLoop->blocks()) {
     // We don't support switch statements inside loops.
     if (!isa<BranchInst>(BB->getTerminator())) {
       reportVectorizationFailure("Loop contains a switch statement",
                                  "loop contains a switch statement",
                                  "LoopContainsSwitch", ORE, TheLoop,
                                  BB->getTerminator());
       return false;
     }
 

llvm/test/Transforms/LoopVectorize/AArch64/sve-scalable-load-in-loop.ll

+; RUN: opt -S -O2 -mtriple=aarch64-linux-gnu -mattr=+sve < %s 2>%t | FileCheck %s
+; RUN: FileCheck --check-prefix=WARN --allow-empty %s <%t
+
+; This test is checking that a scalable load inside a loop does not trigger a
+; TypeSize error in the loop vectorization legality analysis. It is possible for
+; a scalable/vector load to appear inside a loop at vectorization legality
+; analysis if, for example, the ACLE are used. If we encounter a scalable/vector
+; load, it should not be considered for analysis, and we should not see a
+; TypeSize error.
+
+; If this check fails please read test/CodeGen/AArch64/README for instructions on how to resolve it.
+; WARN-NOT: warning: {{.*}}TypeSize is not scalable
+
+; #include <arm_sve.h>
+;
+; void scalable_load_in_loop(long n, int *a, int *b, svuint32_t *x,
+;                            svuint32_t *y) {
+;     for (unsigned i = 0; i < n; i++) {
+;         if (i % 2 == 0) continue;
+;         a[i] = 2 * b[i];
+;         *x = *y;
+;     }
+; }
+
+; CHECK-LABEL: @scalable_load_in_loop
+; CHECK-NOT: vector.body
+define void @scalable_load_in_loop(i64 %n, <vscale x 4 x i32>* %x, <vscale x 4 x i32>* %y) {
+entry:
+  br label %for.body
+
+for.body:
+  %i = phi i32 [ %inc, %for.inc ], [ 0, %entry ]
+  %rem = and i32 %i, 1
+  %cmp = icmp eq i32 %rem, 0
+  br i1 %cmp, label %for.inc, label %if.end
+
+if.end:
+  %0 = load <vscale x 4 x i32>, <vscale x 4 x i32>* %y
+  store <vscale x 4 x i32> %0, <vscale x 4 x i32>* %x
+  br label %for.inc
+
+for.inc:
+  %inc = add i32 %i, 1
+  %cmp2 = icmp slt i64 0, %n
+  br i1 %cmp2, label %for.body, label %for.cleanup
+
+for.cleanup:
+  ret void
+}