-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[LV] Don't mark ptrs as safe to speculate if fed by UB/poison op. #143204
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1493,10 +1493,51 @@ bool LoopVectorizationLegality::canVectorizeWithIfConvert() { | |
| SmallVector<const SCEVPredicate *, 4> Predicates; | ||
| for (Instruction &I : *BB) { | ||
| LoadInst *LI = dyn_cast<LoadInst>(&I); | ||
|
|
||
| // Make sure we can execute all computations feeding into Ptr in the loop | ||
| // w/o triggering UB and that none of the out-of-loop operands are poison. | ||
| // We do not need to check if operations inside the loop can produce | ||
| // poison due to flags (e.g. due to an inbounds GEP going out of bounds), | ||
| // because flags will be dropped when executing them unconditionally. | ||
| // TODO: Results could be improved by considering poison-propagation | ||
| // properties of visited ops. | ||
|
Comment on lines
+1500
to
+1501
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Post-commit nit: another TODO is to potentially save compile time by computing all SpeculatablePointers defined in the loop together, instead of analyzing the expression of each pointer independently, as some pointers may be used to define others? |
||
| auto CanSpeculateOp = [this](Value *Ptr) { | ||
|
||
| SmallVector<Value *> Worklist = {Ptr}; | ||
| SmallPtrSet<Value *, 4> Visited; | ||
| while (!Worklist.empty()) { | ||
| Value *CurrV = Worklist.pop_back_val(); | ||
| if (!Visited.insert(CurrV).second) | ||
| continue; | ||
|
|
||
| auto *CurrI = dyn_cast<Instruction>(CurrV); | ||
| if (!CurrI || !TheLoop->contains(CurrI)) { | ||
| // If operands from outside the loop may be poison then Ptr may also | ||
| // be poison. | ||
| if (!isGuaranteedNotToBePoison(CurrV, AC, | ||
| TheLoop->getLoopPredecessor() | ||
| ->getTerminator() | ||
| ->getIterator())) | ||
| return false; | ||
| continue; | ||
| } | ||
|
|
||
| // A loaded value may be poison, independent of any flags. | ||
| if (isa<LoadInst>(CurrI) && !isGuaranteedNotToBePoison(CurrV, AC)) | ||
| return false; | ||
|
|
||
| // For other ops, assume poison can only be introduced via flags, | ||
| // which can be dropped. | ||
| if (!isa<PHINode>(CurrI) && !isSafeToSpeculativelyExecute(CurrI)) | ||
| return false; | ||
| append_range(Worklist, CurrI->operands()); | ||
| } | ||
| return true; | ||
| }; | ||
| // Pass the Predicates pointer to isDereferenceableAndAlignedInLoop so | ||
| // that it will consider loops that need guarding by SCEV checks. The | ||
| // vectoriser will generate these checks if we decide to vectorise. | ||
| if (LI && !LI->getType()->isVectorTy() && !mustSuppressSpeculation(*LI) && | ||
| CanSpeculateOp(LI->getPointerOperand()) && | ||
| isDereferenceableAndAlignedInLoop(LI, TheLoop, SE, *DT, AC, | ||
| &Predicates)) | ||
| SafePointers.insert(LI->getPointerOperand()); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does include things like nsw, nuw for sub/add operations? I'm just thinking of a scalar loop that has something like:
where the
%index_plus_offsetcould be poison-generating. Are you saying that after vectorisation any such flags on adds or subs that feed into the GEP cannot survive?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes it includes all relevant flags, including
nuw,nsw,inbounds. The flags are only dropped for any op that computes a pointer for loads that are executed conditionally in the original loop but executed unconditionally in the vector loop.