Page MenuHomePhabricator

Fix crash in VectorCombine when attempting to peephole ConstantVector sequences
AbandonedPublic

Authored by clin1 on Jun 17 2020, 1:07 PM.

Details

Reviewers
spatel
Summary

@spatel, are you OK with this fix? If Constants are given to foldExtractExtract, this Builder call:

Value *NewExt = Builder.CreateExtractElement(Shuf, CheapExtIndex);

will end up with a Constant instead of an Instruction, causing a crash on the later cast.

Diff Detail

Unit TestsFailed

TimeTest
90 mslinux > LLVM.Transforms/VectorCombine/X86::Unknown Unit Message ("")
Script: -- : 'RUN: at line 2'; /mnt/disks/ssd0/agent/llvm-project/build/bin/opt < /mnt/disks/ssd0/agent/llvm-project/llvm/test/Transforms/VectorCombine/X86/extract-binop.ll -vector-combine -S -mtriple=x86_64-- -mattr=SSE2 | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/Transforms/VectorCombine/X86/extract-binop.ll --check-prefixes=CHECK,SSE
120 mswindows > LLVM.Transforms/VectorCombine/X86::Unknown Unit Message ("")
Script: -- : 'RUN: at line 2'; c:\ws\w6\llvm-project\premerge-checks\build\bin\opt.exe < C:\ws\w6\llvm-project\premerge-checks\llvm\test\Transforms\VectorCombine\X86\extract-binop.ll -vector-combine -S -mtriple=x86_64-- -mattr=SSE2 | c:\ws\w6\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w6\llvm-project\premerge-checks\llvm\test\Transforms\VectorCombine\X86\extract-binop.ll --check-prefixes=CHECK,SSE

Event Timeline

clin1 created this revision.Jun 17 2020, 1:07 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 17 2020, 1:07 PM

Was this found by a fuzzer, or is this part of a real compilation?
Either way, I'm worried that we may find more cases like this because we've generally assumed that code is coming into -vector-combine in mostly optimized form.

Maybe it's better to cheaply pre-process a block before trying anything in this pass? That way, we won't get sidetracked even if something like SLP has left stray code in the function.

diff --git a/llvm/lib/Transforms/Vectorize/VectorCombine.cpp b/llvm/lib/Transforms/Vectorize/VectorCombine.cpp
index 05f9c6f7daf..7c4c61f8bb6 100644
--- a/llvm/lib/Transforms/Vectorize/VectorCombine.cpp
+++ b/llvm/lib/Transforms/Vectorize/VectorCombine.cpp
@@ -445,6 +445,9 @@ static bool runImpl(Function &F, const TargetTransformInfo &TTI,
     // Ignore unreachable basic blocks.
     if (!DT.isReachableFromEntry(&BB))
       continue;
+
+    MadeChange |= SimplifyInstructionsInBlock(&BB);
+
     // Do not delete instructions under here and invalidate the iterator.
     // Walk the block forwards to enable simple iterative chains of transforms.
     // TODO: It could be more efficient to remove dead instructions
llvm/test/Transforms/VectorCombine/X86/ignore-const.ll
12 ↗(On Diff #271458)

We can reduce the test to something like this:

define i32 @constant_fold_crash(<4 x i32> %x) {
  %c = extractelement <4 x i32> <i32 16, i32 17, i32 18, i32 19>, i32 1
  %d = extractelement <4 x i32> %x, i64 0
  %e = add i32 %c, %d
  ret i32 %e
}
clin1 added a comment.Jun 17 2020, 3:58 PM

If running IC is cheap, that's a good solution. I will put up another patch. Thanks.
The IR looks strange, but it's unfortunately showing up in a few real test cases in our LLVM branch, after we've made some mods to the unroller and vectorizer.

clin1 updated this revision to Diff 271590.Jun 18 2020, 12:07 AM
clin1 edited the summary of this revision. (Show Details)

Updated to run InstCombine before the vector transformations. The insert-binop-with-constant test needed a few changes. InstructionSimplify checks for constant divisors with 0/undef elements and undef's the entire result.

Updated to run InstCombine before the vector transformations. The insert-binop-with-constant test needed a few changes. InstructionSimplify checks for constant divisors with 0/undef elements and undef's the entire result.

We are not running the entire InstCombine here - that could be expensive. This is more like running InstSimplify. The changes to the other tests are good; they show that VectorCombine is not losing the ideal optimizations on those patterns.

I think this patch is almost ready, but please adjust the test and upload with full context ("-U9999"):
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

llvm/test/Transforms/VectorCombine/X86/fold-extract.ll
11 ↗(On Diff #271590)

Please remove unnecessary text:
Function Attrs comment, dso_local, local_unnamed_addr, #0 (and corresponding attributes line).

I think you can just add this test to the existing extract-binop.ll test file instead of adding a new file.

clin1 updated this revision to Diff 271821.Jun 18 2020, 1:12 PM

Good clarification: InstCombine vs. InstSimplify.
Merged and reduced test, full diff included.
If the patch is OK, could I ask you one more favor: to commit it? I do not have commit access to the project.

Good clarification: InstCombine vs. InstSimplify.
Merged and reduced test, full diff included.
If the patch is OK, could I ask you one more favor: to commit it? I do not have commit access to the project.

Sure I can commit for you.
But I just noticed a potential inefficiency, so I think we should make a logic adjustment. If we succeed at the preliminary cleanup, then we will run SimplifyInstructionsInBlock again on exit even if the main part of this pass didn't make changes.
It would be better if we make those independent with something like this:

bool MadeCleanupChange = false;
bool MadeVectorChange = false;
...
MadeCleanupChange |= SimplifyInstructionsInBlock(&BB);
...
MadeVectorChange |= foldExtractExtract(I, TTI);
...
if (MadeVectorChange) { do more cleanup }
...
return MadeCleanupChange | MadeVectorChange;
nikic added a subscriber: nikic.Jun 19 2020, 7:41 AM

Compile-time numbers: https://llvm-compile-time-tracker.com/compare.php?from=35651fdd4537c081ca47acde58b6fd4b5f6997b3&to=5ff2db86f51083878ff9a458a39b355a03b31e73&stat=instructions

It should be noted that while InstSimplify is cheap relative to InstCombine, it's likely a good bit more expansive than the actual VectorCombine itself :)

As the original patch does not look particularly complicated, I would prefer the more targeted fix.

Thanks for pre-testing that!

It should be noted that while InstSimplify is cheap relative to InstCombine, it's likely a good bit more expansive than the actual VectorCombine itself :)

I figured that InstSimplify probably costs more than this pass, but I didn't realize it would be above the noise in a full compile. Good to know.

As the original patch does not look particularly complicated, I would prefer the more targeted fix.

Ok - let's go back to the 1st draft with the updated test.

I am looking at refactoring some of this code, so I pushed a similar fix here:
rG6d86409
So I think it's ok to abandon this patch.

Let me know if that solves all of the known crashing cases.

clin1 updated this revision to Diff 272115.Jun 19 2020, 9:54 AM

Thanks for the consensus. New patch uploaded.

clin1 updated this revision to Diff 272122.Jun 19 2020, 10:10 AM

Return true to trigger the simplifier.

Ah, I missed your comment. I'll pull your fix and retest.

clin1 abandoned this revision.Jun 19 2020, 3:33 PM

Our internal test passes---thank you for the fix.

Our internal test passes---thank you for the fix.

Thanks for spotting the bug! If you still want to make the change to 'return true' if we find a constant folding opportunity, I think that would be fine to do. But we should change the variable name from 'MadeChange' to something like 'NeedsCleanup' to not be confusing.