[PATCH] D77808: [SCCP] Use conditional info with AND/OR branch conditions.

Tue Jul 7 12:12:12 PDT 2020

fhahn marked an inline comment as done.
fhahn added inline comments.

================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:1320
+        else
+          NewCR = NewCR.intersectWith(CopyOfCR);

----------------
nikic wrote:
> I'd write this as:
> ```
> NewCR = NewCR.intersectWith(CopyOfCR);
> if (!CopyOfCR.contains(NewCR) &&
>     CopyOfCR.getSingleMissingElement() &&
>     CopyOf != OriginalVal)
>   NewCR = CopyOfCR;
> ```
> There is no need to discard the intersection if it is smaller than the original range. E.g. if you had information for `ne 0` before, then intersecting it with `ugt 42` will always be beneficial.
> 
> I'm also not clear on why the `CopyOf != OriginalVal` comparison is there. If we think that inequality information is more valuable, shouldn't that hold regardless of how we arrived at the inequality?
Thanks, that's simpler indeed. I think the impact in practice is relatively low, but it is definitely more straight-forward:) 

> I'm also not clear on why the CopyOf != OriginalVal comparison is there
It was mainly there to guard against specific potential regressions caused by this patch. But it is probably not worth special casing here. I've remove the check.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D77808/new/

https://reviews.llvm.org/D77808