[PATCH] D43536: [LV] Quick workaround for PR36311, vectorizer's isUniform() abuse triggers assert in SCEV

[Hideki Saito via Phabricator via llvm-commits]

hsaito created this revision.
hsaito added reviewers: Meinersbur, skatkov, mkazantsev, Ayal, mkuper, hfinkel, rengolin.
Herald added a subscriber: llvm-commits.

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.

>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 works around the assert in the SCEV by simply commenting out the (useless) assert in LV in case someone needs immediate action. Real solution to PR36311 should come from fixing the architectural problem --- guesstimated ETA is 3 weeks.


Repository:
  rL LLVM

https://reviews.llvm.org/D43536

Files:
  lib/Transforms/Vectorize/LoopVectorize.cpp


Index: lib/Transforms/Vectorize/LoopVectorize.cpp
===================================================================
--- lib/Transforms/Vectorize/LoopVectorize.cpp
+++ lib/Transforms/Vectorize/LoopVectorize.cpp
@@ -3048,8 +3048,13 @@
 
   // Handle Stores:
   if (SI) {
-    assert(!Legal->isUniform(SI->getPointerOperand()) &&
-           "We do not allow storing to uniform addresses");
+    // TODO
+    // Quick workaround for PR36311 by commenting out the assert for which
+    // Legality already ensured will never trigger. Real fix is to avoid
+    // recomputing isUniform() here. Eventually, we should support some form
+    // of stores to uniform addresses.
+    // assert(!Legal->isUniform(SI->getPointerOperand()) &&
+    //        "We do not allow storing to uniform addresses");
     setDebugLocFromInst(Builder, SI);
 
     for (unsigned Part = 0; Part < UF; ++Part) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D43536.135164.patch
Type: text/x-patch
Size: 887 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180221/4ec16dae/attachment.bin>