[PATCH] D140260: [LoopVectorize] Fix crash on "vector->scalar" bitcast vectorization
Florian Hahn via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Jan 8 06:46:20 PST 2023
fhahn added inline comments.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4736
+ if (isa<BitCastInst>(&I) && I.getOperand(0)->getType()->isVectorTy()) {
+ assert(TheLoop->hasLoopInvariantOperands(&I) &&
+ "Vector operand must be loop-invariant");
----------------
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.
================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:4738
+ "Vector operand must be loop-invariant");
+ LLVM_DEBUG(dbgs() << "LV: Found uniform instruction: " << I << "\n");
+ addToWorklistIfAllowed(&I);
----------------
No need to dump there I think, the message will be printed once the entry in the worklist has been processed.
================
Comment at: llvm/test/Transforms/LoopVectorize/vector2scalar-bitcast.ll:1
+; REQUIRES: assert
+; RUN: opt < %s -loop-vectorize -force-vector-width=2 -S 2>&1 | FileCheck %s
----------------
the test doesn't requires asserts.
================
Comment at: llvm/test/Transforms/LoopVectorize/vector2scalar-bitcast.ll:2
+; REQUIRES: assert
+; RUN: opt < %s -loop-vectorize -force-vector-width=2 -S 2>&1 | FileCheck %s
+
----------------
This is missing CHECK lines? Should probably check at least the vector code generated.
================
Comment at: llvm/test/Transforms/LoopVectorize/vector2scalar-bitcast.ll:4
+
+; Function Attrs: argmemonly nofree norecurse nosync nounwind uwtable
+define dso_local i64 @foo(ptr nocapture noundef %a, i32 noundef %n, <2 x float> %in2) {
----------------
not needed
================
Comment at: llvm/test/Transforms/LoopVectorize/vector2scalar-bitcast.ll:5
+; 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:
----------------
dso_local not needed.
================
Comment at: llvm/test/Transforms/LoopVectorize/vector2scalar-bitcast.ll:8
+ %cmp3 = icmp sgt i32 %n, 0
+ br i1 %cmp3, label %for.body.preheader, label %for.cond.cleanup
+
----------------
those checks should not be needed
================
Comment at: llvm/test/Transforms/LoopVectorize/vector2scalar-bitcast.ll:11
+for.body.preheader: ; preds = %entry
+ %wide.trip.count = zext i32 %n to i64
+ br label %for.body
----------------
no need for the zext here, could either use constant or pass `%n` as i64.
================
Comment at: llvm/test/Transforms/LoopVectorize/vector2scalar-bitcast.ll:14
+
+for.cond.cleanup: ; preds = %for.body, %entry
+ %result = phi i64 [ 0, %entry], [ %illegal, %for.body]
----------------
for easier readability, move the block to the end and call something like `exit:`?
================
Comment at: llvm/test/Transforms/LoopVectorize/vector2scalar-bitcast.ll:19
+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
----------------
`indvars.*` prefix makes the names unnecessarily long, could be dropped.
================
Comment at: llvm/test/Transforms/LoopVectorize/vector2scalar-bitcast.ll:21
+ %arrayidx = getelementptr inbounds i32, ptr %a, i64 %indvars.iv
+ %illegal = bitcast <2 x float> %in2 to i64
+ %0 = load i32, ptr %arrayidx, align 4
----------------
Could you also add a variant where %illegal is used in the loop?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140260/new/
https://reviews.llvm.org/D140260
More information about the llvm-commits
mailing list