[LoopVectorize] Fix crash on "vector->scalar" bitcast vectorization

[mdchen]

Summary

It's possible for a scalar bitcast takes vector input and the current
LoopVectorize cannot manage it correctly and crash. Because a scalar
loop only contains scalar instructions that cannot update vectors, the
first vector operand of such bitcasts must be loop-invariant and should
be uniform.

[mdchen]

Test checks will be updated later.

[lebedev.ri]

It think the loop-invariantness precondition for these is overly restrictive.

[mdchen]

In D140260#4003727, @lebedev.ri wrote:

It think the loop-invariantness precondition for these is overly restrictive.

Any suggestion on that? IIUC there's also some inconsistency as the current legality check fails if ExtractElementInst is found:

// Check that the instruction return type is vectorizable.
// Also, we can't vectorize extractelement instructions.
if ((!VectorType::isValidElementType(I.getType()) &&
     !I.getType()->isVoidTy()) ||
    isa<ExtractElementInst>(I)) {
  reportVectorizationFailure("Found unvectorizable type",
      "instruction return type cannot be vectorized",
      "CantVectorizeInstructionReturnType", ORE, TheLoop, &I);
  return false;
}

which I suppose is uniform.

[lebedev.ri]

In D140260#4004038, @mdchen wrote:

In D140260#4003727, @lebedev.ri wrote:

It think the loop-invariantness precondition for these is overly restrictive.

Any suggestion on that?

For context. loading i64 and loading <8 x i8> and bitcasting to i64 should be the same thing.

IIUC there's also some inconsistency as the current legality check fails if ExtractElementInst is found:

// Check that the instruction return type is vectorizable.
// Also, we can't vectorize extractelement instructions.
if ((!VectorType::isValidElementType(I.getType()) &&
     !I.getType()->isVoidTy()) ||
    isa<ExtractElementInst>(I)) {
  reportVectorizationFailure("Found unvectorizable type",
      "instruction return type cannot be vectorized",
      "CantVectorizeInstructionReturnType", ORE, TheLoop, &I);
  return false;
}

which I suppose is uniform.

I'm not really familiar with this area, so i can't quite help with this.

[mdchen]

In D140260#4004726, @lebedev.ri wrote:

In D140260#4004038, @mdchen wrote:

In D140260#4003727, @lebedev.ri wrote:

It think the loop-invariantness precondition for these is overly restrictive.

Any suggestion on that?

For context. loading i64 and loading <8 x i8> and bitcasting to i64 should be the same thing.

Let me try to catch your point, the key difference here is loading <8 x i8> cannot appear in a scalar loop and the legality check will ensure that. Thus the vector operand of bitcast must be defined outside the loop.

[mdchen]

ping

[mdchen]

ping again:)

[fhahn]

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

4736

This should use isOutOfScope like the ExtractValueInst case?

It would also be good to add a test case where the vector bit cast is loop-dependent. At the moment, we bail out on any instruction with a vector return type, but it would be good to have a case if the restriction gets removed some day.

4738

No need to dump there I think, the message will be printed once the entry in the worklist has been processed.

llvm/test/Transforms/LoopVectorize/vector2scalar-bitcast.ll

1

the test doesn't requires asserts.

2

This is missing CHECK lines? Should probably check at least the vector code generated.

4

not needed

5

dso_local not needed.

8

those checks should not be needed

11

no need for the zext here, could either use constant or pass %n as i64.

14

for easier readability, move the block to the end and call something like exit:?

19

indvars.* prefix makes the names unnecessarily long, could be dropped.

21

Could you also add a variant where %illegal is used in the loop?

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp

for (auto &I : *BB) {
           if (TheLoop->hasLoopInvariantOperands(&I))
             addToWorklistIfAllowed(&I);
           break;
         default:
           break;
         }
       }
 
+      // When the first operand of a bitcast instruction is of vector type, its
+      // vector operand must be loop-invariant and the bitcast must be uniform.
+      if (isa<BitCastInst>(&I) && I.getOperand(0)->getType()->isVectorTy()) {
+        assert(TheLoop->hasLoopInvariantOperands(&I) &&
+               "Vector operand must be loop-invariant");
+        LLVM_DEBUG(dbgs() << "LV: Found uniform instruction: " << I << "\n");
+        addToWorklistIfAllowed(&I);
+        continue;
+      }
+
       // ExtractValue instructions must be uniform, because the operands are
       // known to be loop-invariant.
       if (auto *EVI = dyn_cast<ExtractValueInst>(&I)) {
         assert(isOutOfScope(EVI->getAggregateOperand()) &&
                "Expected aggregate value to be loop invariant");
         addToWorklistIfAllowed(EVI);
         continue;
       }

llvm/test/Transforms/LoopVectorize/vector2scalar-bitcast.ll

+; REQUIRES: assert
+; RUN: opt < %s -loop-vectorize -force-vector-width=2 -S 2>&1 | FileCheck %s
+
+; Function Attrs: argmemonly nofree norecurse nosync nounwind uwtable
+define dso_local i64 @foo(ptr nocapture noundef %a, i32 noundef %n, <2 x float> %in2) {
+entry:
+  %cmp3 = icmp sgt i32 %n, 0
+  br i1 %cmp3, label %for.body.preheader, label %for.cond.cleanup
+
+for.body.preheader:                               ; preds = %entry
+  %wide.trip.count = zext i32 %n to i64
+  br label %for.body
+
+for.cond.cleanup:                                 ; preds = %for.body, %entry
+  %result = phi i64 [ 0, %entry], [ %illegal, %for.body]
+  ret i64 %result
+
+for.body:                                         ; preds = %for.body.preheader, %for.body
+  %indvars.iv = phi i64 [ 0, %for.body.preheader ], [ %indvars.iv.next, %for.body ]
+  %arrayidx = getelementptr inbounds i32, ptr %a, i64 %indvars.iv
+  %illegal = bitcast <2 x float> %in2 to i64
+  %0 = load i32, ptr %arrayidx, align 4
+  %add = add nsw i32 %0, 1
+  store i32 %add, ptr %arrayidx, align 4
+  %indvars.iv.next = add nuw nsw i64 %indvars.iv, 1
+  %exitcond.not = icmp eq i64 %indvars.iv.next, %wide.trip.count
+  br i1 %exitcond.not, label %for.cond.cleanup, label %for.body
+}
+
+