[llvm] 493bab8 - [NFC][SCEV] Reflow `impliesPoison()` into an exhaustive switch

[Nikita Popov via llvm-commits]

On Sun, Jan 22, 2023, at 16:57, Roman Lebedev via llvm-commits wrote:
> 
> Author: Roman Lebedev
> Date: 2023-01-22T18:57:22+03:00
> New Revision: 493bab8867bf17f54ccb9466a12622604287ad07
> 
> URL: https://github.com/llvm/llvm-project/commit/493bab8867bf17f54ccb9466a12622604287ad07
> DIFF: https://github.com/llvm/llvm-project/commit/493bab8867bf17f54ccb9466a12622604287ad07.diff
> 
> LOG: [NFC][SCEV] Reflow `impliesPoison()` into an exhaustive switch
> 
> Added: 
>     
> 
> Modified: 
>     llvm/lib/Analysis/ScalarEvolution.cpp
> 
> Removed: 
>     
> 
> 
> ################################################################################
> diff  --git a/llvm/lib/Analysis/ScalarEvolution.cpp b/llvm/lib/Analysis/ScalarEvolution.cpp
> index 4c1491b4c0c3..9025b90868d6 100644
> --- a/llvm/lib/Analysis/ScalarEvolution.cpp
> +++ b/llvm/lib/Analysis/ScalarEvolution.cpp
> @@ -4094,6 +4094,33 @@ class SCEVSequentialMinMaxDeduplicatingVisitor final
>  
> } // namespace
>  
> +static bool scevUnconditionallyPropagatesPoisonFromOperands(SCEVTypes Kind) {
> +  switch (Kind) {
> +  case scConstant:
> +  case scTruncate:
> +  case scZeroExtend:
> +  case scSignExtend:
> +  case scPtrToInt:
> +  case scAddExpr:
> +  case scMulExpr:
> +  case scUDivExpr:
> +  case scAddRecExpr:
> +  case scUMaxExpr:
> +  case scSMaxExpr:
> +  case scUMinExpr:
> +  case scSMinExpr:
> +  case scUnknown:
> +    // If any operand is poison, the whole expression is poison.
> +    return true;
> +  case scSequentialUMinExpr:
> +    // FIXME: if the *first* operand is poison, the whole expression is poison.
> +    return false; // Pessimistically, say that it does not propagate poison.
> +  case scCouldNotCompute:
> +    llvm_unreachable("Attempt to use a SCEVCouldNotCompute object!");
> +  }
> +  llvm_unreachable("Unknown SCEV kind!");
> +}
> +
> /// Return true if V is poison given that AssumedPoison is already poison.
> static bool impliesPoison(const SCEV *AssumedPoison, const SCEV *S) {
>    // The only way poison may be introduced in a SCEV expression is from a
> @@ -4110,10 +4137,33 @@ static bool impliesPoison(const SCEV *AssumedPoison, const SCEV *S) {
>      SCEVPoisonCollector(bool LookThroughSeq) : LookThroughSeq(LookThroughSeq) {}
>  
>      bool follow(const SCEV *S) {
> -      // TODO: We can always follow the first operand, but the SCEVTraversal
> -      // API doesn't support this.
> -      if (!LookThroughSeq && isa<SCEVSequentialMinMaxExpr>(S))
> -        return false;
> +      if (!scevUnconditionallyPropagatesPoisonFromOperands(S->getSCEVType())) {
> +        switch (S->getSCEVType()) {
> +        case scConstant:
> +        case scTruncate:
> +        case scZeroExtend:
> +        case scSignExtend:
> +        case scPtrToInt:
> +        case scAddExpr:
> +        case scMulExpr:
> +        case scUDivExpr:
> +        case scAddRecExpr:
> +        case scUMaxExpr:
> +        case scSMaxExpr:
> +        case scUMinExpr:
> +        case scSMinExpr:
> +        case scUnknown:
> +          llvm_unreachable("These all unconditionally propagate poison.");
> +        case scSequentialUMinExpr:
> +          // TODO: We can always follow the first operand,
> +          // but the SCEVTraversal API doesn't support this.
> +          if (!LookThroughSeq)
> +            return false;
> +          break;
> +        case scCouldNotCompute:
> +          llvm_unreachable("Attempt to use a SCEVCouldNotCompute object!");
> +        }
> +      }
>  
>        if (auto *SU = dyn_cast<SCEVUnknown>(S)) {
>          if (!isGuaranteedNotToBePoison(SU->getValue()))

Per Romans request, cross-posting my feedback from the GitHub commit (https://github.com/llvm/llvm-project/commit/493bab8867bf17f54ccb9466a12622604287ad07#r97687549) here:

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.

Regards,
Nikita
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230123/a8b81261/attachment.html>