<!DOCTYPE html><html><head><title></title><style type="text/css">p.MsoNormal,p.MsoNoSpacing{margin:0}</style></head><body><div>On Sun, Jan 22, 2023, at 16:57, Roman Lebedev via llvm-commits wrote:<br></div><blockquote type="cite" id="qt" style=""><div><br></div><div>Author: Roman Lebedev<br></div><div>Date: 2023-01-22T18:57:22+03:00<br></div><div>New Revision: 493bab8867bf17f54ccb9466a12622604287ad07<br></div><div><br></div><div>URL: <a href="https://github.com/llvm/llvm-project/commit/493bab8867bf17f54ccb9466a12622604287ad07">https://github.com/llvm/llvm-project/commit/493bab8867bf17f54ccb9466a12622604287ad07</a><br></div><div>DIFF: <a href="https://github.com/llvm/llvm-project/commit/493bab8867bf17f54ccb9466a12622604287ad07.diff">https://github.com/llvm/llvm-project/commit/493bab8867bf17f54ccb9466a12622604287ad07.diff</a><br></div><div><br></div><div>LOG: [NFC][SCEV] Reflow `impliesPoison()` into an exhaustive switch<br></div><div><br></div><div>Added: <br></div><div> <br></div><div><br></div><div>Modified: <br></div><div> llvm/lib/Analysis/ScalarEvolution.cpp<br></div><div><br></div><div>Removed: <br></div><div> <br></div><div><br></div><div><br></div><div>################################################################################<br></div><div>diff --git a/llvm/lib/Analysis/ScalarEvolution.cpp b/llvm/lib/Analysis/ScalarEvolution.cpp<br></div><div>index 4c1491b4c0c3..9025b90868d6 100644<br></div><div>--- a/llvm/lib/Analysis/ScalarEvolution.cpp<br></div><div>+++ b/llvm/lib/Analysis/ScalarEvolution.cpp<br></div><div>@@ -4094,6 +4094,33 @@ class SCEVSequentialMinMaxDeduplicatingVisitor final<br></div><div> <br></div><div>} // namespace<br></div><div> <br></div><div>+static bool scevUnconditionallyPropagatesPoisonFromOperands(SCEVTypes Kind) {<br></div><div>+ switch (Kind) {<br></div><div>+ case scConstant:<br></div><div>+ case scTruncate:<br></div><div>+ case scZeroExtend:<br></div><div>+ case scSignExtend:<br></div><div>+ case scPtrToInt:<br></div><div>+ case scAddExpr:<br></div><div>+ case scMulExpr:<br></div><div>+ case scUDivExpr:<br></div><div>+ case scAddRecExpr:<br></div><div>+ case scUMaxExpr:<br></div><div>+ case scSMaxExpr:<br></div><div>+ case scUMinExpr:<br></div><div>+ case scSMinExpr:<br></div><div>+ case scUnknown:<br></div><div>+ // If any operand is poison, the whole expression is poison.<br></div><div>+ return true;<br></div><div>+ case scSequentialUMinExpr:<br></div><div>+ // FIXME: if the *first* operand is poison, the whole expression is poison.<br></div><div>+ return false; // Pessimistically, say that it does not propagate poison.<br></div><div>+ case scCouldNotCompute:<br></div><div>+ llvm_unreachable("Attempt to use a SCEVCouldNotCompute object!");<br></div><div>+ }<br></div><div>+ llvm_unreachable("Unknown SCEV kind!");<br></div><div>+}<br></div><div>+<br></div><div>/// Return true if V is poison given that AssumedPoison is already poison.<br></div><div>static bool impliesPoison(const SCEV *AssumedPoison, const SCEV *S) {<br></div><div> // The only way poison may be introduced in a SCEV expression is from a<br></div><div>@@ -4110,10 +4137,33 @@ static bool impliesPoison(const SCEV *AssumedPoison, const SCEV *S) {<br></div><div> SCEVPoisonCollector(bool LookThroughSeq) : LookThroughSeq(LookThroughSeq) {}<br></div><div> <br></div><div> bool follow(const SCEV *S) {<br></div><div>- // TODO: We can always follow the first operand, but the SCEVTraversal<br></div><div>- // API doesn't support this.<br></div><div>- if (!LookThroughSeq && isa<SCEVSequentialMinMaxExpr>(S))<br></div><div>- return false;<br></div><div>+ if (!scevUnconditionallyPropagatesPoisonFromOperands(S->getSCEVType())) {<br></div><div>+ switch (S->getSCEVType()) {<br></div><div>+ case scConstant:<br></div><div>+ case scTruncate:<br></div><div>+ case scZeroExtend:<br></div><div>+ case scSignExtend:<br></div><div>+ case scPtrToInt:<br></div><div>+ case scAddExpr:<br></div><div>+ case scMulExpr:<br></div><div>+ case scUDivExpr:<br></div><div>+ case scAddRecExpr:<br></div><div>+ case scUMaxExpr:<br></div><div>+ case scSMaxExpr:<br></div><div>+ case scUMinExpr:<br></div><div>+ case scSMinExpr:<br></div><div>+ case scUnknown:<br></div><div>+ llvm_unreachable("These all unconditionally propagate poison.");<br></div><div>+ case scSequentialUMinExpr:<br></div><div>+ // TODO: We can always follow the first operand,<br></div><div>+ // but the SCEVTraversal API doesn't support this.<br></div><div>+ if (!LookThroughSeq)<br></div><div>+ return false;<br></div><div>+ break;<br></div><div>+ case scCouldNotCompute:<br></div><div>+ llvm_unreachable("Attempt to use a SCEVCouldNotCompute object!");<br></div><div>+ }<br></div><div>+ }<br></div><div> <br></div><div> if (auto *SU = dyn_cast<SCEVUnknown>(S)) {<br></div><div> if (!isGuaranteedNotToBePoison(SU->getValue()))<br></div></blockquote><div><br></div><div>Per Romans request, cross-posting my feedback from the GitHub commit (<a href="https://github.com/llvm/llvm-project/commit/493bab8867bf17f54ccb9466a12622604287ad07#r97687549">https://github.com/llvm/llvm-project/commit/493bab8867bf17f54ccb9466a12622604287ad07#r97687549</a>) here:<br></div><div><br></div><div>The addition of scevUnconditionallyPropagatesPoisonFromOperands() with an exhaustive switch is fine. The second switch should not be necessary. Can't you simply return false if !scevUnconditionallyPropagatesPoisonFromOperands() && !LookThroughSeq? At least if you rename LookThroughSeq to match the more generic check.<br></div><div><br></div><div>Regards,<br></div><div>Nikita<br></div></body></html>