-
Notifications
You must be signed in to change notification settings - Fork 13.7k
[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
base: main
Are you sure you want to change the base?
Changes from all commits
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. | ||
auto CanSpeculateOp = [this](Value *Ptr) { | ||
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. This function is specifically related to UB, so perhaps worth making it a bit clearer by adding it to the name, i.e. 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. It checks both for instructions that trigger immediate UB when computing the pointer and cases where the pointer itself may be poison, thus triggering UB if dereferenced, which is why I tried to keep the name generic, but it's not 100% clear at the moment. 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. OK, maybe something more specific to pointers then as it gives the impression this is about any operation. Perhaps |
||
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_offset
could 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.