[LV] Fix for PR36311, vectorizer's isUniform() abuse triggers assert in SCEV

[hsaito]

Summary

See more detailed analysis in https://bugs.llvm.org/show_bug.cgi?id=36311.

isUniform() information is recomputed after LV started transforming the underlying IR and that triggered an assert in SCEV.

From vectorizer's architectural perspective, such information, while still useful in vector code gen, should not be recomputed after the start of transforming the LLVM IR. Instead, we should collect and cache such information during the analysis phase of LV and use the cached info during code gen.

From the symptom perspective, this assert as it stands right now is not very useful. Legality already rejected loops that would trigger the assert. As such, commenting out the assert is NFC from vectorizer's functionality perspective. On top of that, just above the assertion, we check for unit-strided load/store or
gather scatter. Addresses can't be uniform below that check.

From vectorization theory point of view, we don't have to reject all cases of stores to uniform addresses. Eventually, we should support safe/profitable cases.

This patch resolves the issue by removing the useless assertion that is invoking LAA's isUniform() that requires up-to-date DomTree ---- once vector code gen starts modifying CFG, we don't have an up-to-date DomTree.

[Meinersbur]

Is it worthwhile to add a regression test that would trigger the assertion?

[hsaito]

In D43536#1013881, @Meinersbur wrote:

Is it worthwhile to add a regression test that would trigger the assertion?

Looks like I can just clean up the test cases attached to Bugzilla. Will take care of it later today.

[hsaito]

Added the test that triggered the PR36311 assertion.

[Meinersbur]

LGTM, however I am not the right person to approve patches to the LoopVectorizer.

[skatkov]

SGTM, the same disclaimer :) I'm not a very good at LoopVectorizer :)

[mkazantsev]

test/Transforms/LoopVectorize/pr36311.ll

12

Could you please rename this one into @test() and add a comment what does it check?

[mkazantsev]

LGTM with comments addressed the same disclaimer: I'm not the right person to approve patches in LV.

lib/Transforms/Vectorize/LoopVectorize.cpp

3050–3051

I'd rather prefer FIXME here.

[hsaito]

test/Transforms/LoopVectorize/pr36311.ll

12

How about a comment like this?

; This is a test case that let's vectorizer's code gen to modify CFG and get DomTree out of date,
; such that an assert from SCEV would trigger if reanalysis of SCEV happens subsequently.
; Once vector code gen starts, vectorizer should not invoke recomputation of Analysis.

[rengolin]

Please attach this quick fix to the bug itself, so that people can use it while we fix the problem in the right place. We normally don't comment out code in LLVM, as that tends to leave code in comments all over the place.

[hsaito]

In D43536#1021229, @rengolin wrote:

Please attach this quick fix to the bug itself, so that people can use it while we fix the problem in the right place. We normally don't comment out code in LLVM, as that tends to leave code in comments all over the place.

Fair enough. Done attaching to the bug. We should then just get rid of this useless assert and be done with it. Just above, we have the following assertion, checking for unit-stride or gather/scatter.
Why bother check again for non-uniform address?, and this is on top of Legal already rejecting store to uniform address already.

// Either Ptr feeds a vector load/store, or a vector GEP should feed a vector  
// gather/scatter. Otherwise Decision should have been to Scalarize.           
assert((ConsecutiveStride || CreateGatherScatter) && 
       "The instruction should be scalarized");

Also thought about creating a isUniform() functionality that can be invoked during vector code gen. Bulk of functionality should be already covered by
CostModel::isUniformAfterVectorization() ---- but calling such a thing after checking for the "ConsecutiveStride or GatherScatter" is rather odd.
I'm planning to do another architectural clean up, but using this bug as an excuse to do that would be a poor reasoning.

[hsaito]

lib/Transforms/Vectorize/LoopVectorize.cpp

3050–3051

Changed the comment such that it is actually a TODO for how one can enable store to uniform addr.

[rengolin]

Fair enough, I can't see what case this is trying to cover anyway. I'm happy with the line going out without a big comment, commit message is usually ok.

Sorry it took so long, I'm swapping a bit in the past few weeks.

lib/Transforms/Vectorize/LoopVectorize.cpp

3050–3051

This TODO is confusing. I'm not sure what's the thing we want to do in the future...

Maybe adding that big description on the commit message and a short message for the exact thing we want to implement here, for example, "detect cases where..."

[hsaito]

In D43536#1030857, @rengolin wrote:

Fair enough, I can't see what case this is trying to cover anyway. I'm happy with the line going out without a big comment, commit message is usually ok.

Sorry it took so long, I'm swapping a bit in the past few weeks.

I'll then remove the TODO comment and update the commit message.

[rengolin]

LGTM, thanks!

[hsaito]

Great. Like my other patches, I don't have a commit privilege. Please commit this for me.

Thanks,
Hideki

[rengolin]

r327109

lib/Transforms/Vectorize/LoopVectorize.cpp

if (ConsecutiveStride)
     Ptr = getOrCreateScalarValue(Ptr, {0, 0});
 
   VectorParts Mask;
   bool isMaskRequired = BlockInMask;
   if (isMaskRequired)
     Mask = *BlockInMask;
 
   // Handle Stores:
   if (SI) {
-    assert(!Legal->isUniform(SI->getPointerOperand()) &&
-           "We do not allow storing to uniform addresses");
     setDebugLocFromInst(Builder, SI);
 
     for (unsigned Part = 0; Part < UF; ++Part) {
       Instruction *NewSI = nullptr;
       Value *StoredVal = getOrCreateVectorValue(SI->getValueOperand(), Part);
       if (CreateGatherScatter) {
         Value *MaskPart = isMaskRequired ? Mask[Part] : nullptr;
         Value *VectorGep = getOrCreateVectorValue(Ptr, Part);
         NewSI = Builder.CreateMaskedScatter(StoredVal, VectorGep, Alignment,

test/Transforms/LoopVectorize/pr36311.ll

+; RUN: opt -loop-vectorize -force-vector-width=2 -S < %s
+;
+; Cleaned up version of fe_tools.all_dimensions.ll from PR36311.
+; Forcing VF=2 to trigger vector code gen
+;
+; This is a test case that let's vectorizer's code gen to modify CFG and get
+; DomTree out of date, such that an assert from SCEV would trigger if
+; reanalysis of SCEV happens subsequently. Once vector code gen starts,
+; vectorizer should not invoke recomputation of Analysis.
+
+$test = comdat any
+
+declare i32 @__gxx_personality_v0(...)
+
+; Function Attrs: uwtable
+define dso_local void @test() local_unnamed_addr #0 comdat align 2 personality i8* bitcast (i32 (...)* @__gxx_personality_v0 to i8*) {
+entry:
+  br label %for.body51
+
+for.body51:                                       ; preds = %for.body51, %entry
+  br i1 undef, label %for.body51, label %for.body89.lr.ph
+
+for.cond80.loopexit:                              ; preds = %for.body89
+  %inc94.lcssa = phi i32 [ %inc94, %for.body89 ]
+  br i1 undef, label %for.body89.lr.ph, label %nrvo.skipdtor.loopexit
+
+for.body89.lr.ph:                                 ; preds = %for.cond80.loopexit, %for.body51
+  %i79.0179 = phi i32 [ %add90, %for.cond80.loopexit ], [ 0, %for.body51 ]
+  %next_index.4178 = phi i32 [ %inc94.lcssa, %for.cond80.loopexit ], [ undef, %for.body51 ]
+  %add90 = add nuw i32 %i79.0179, 1
+  %mul91 = mul i32 %add90, undef
+  br label %for.body89
+
+for.body89:                                       ; preds = %for.body89, %for.body89.lr.ph
+  %j.0175 = phi i32 [ 0, %for.body89.lr.ph ], [ %add92, %for.body89 ]
+  %next_index.5174 = phi i32 [ %next_index.4178, %for.body89.lr.ph ], [ %inc94, %for.body89 ]
+  %add92 = add nuw i32 %j.0175, 1
+  %add93 = add i32 %add92, %mul91
+  %inc94 = add i32 %next_index.5174, 1
+  %conv95 = zext i32 %next_index.5174 to i64
+  %arrayidx.i160 = getelementptr inbounds i32, i32* undef, i64 %conv95
+  store i32 %add93, i32* %arrayidx.i160, align 4
+;, !tbaa !1
+  %cmp87 = icmp ult i32 %add92, undef
+  br i1 %cmp87, label %for.body89, label %for.cond80.loopexit
+
+nrvo.skipdtor.loopexit:                           ; preds = %for.cond80.loopexit
+  ret void
+}