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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:1249
+      // out on some cases we could optimize.
+      if (CmpOp0 != OriginalVal && CmpOp0 != CopyOf && CmpOp1 != OriginalVal &&
+          CmpOp1 != CopyOf) {
----------------
fhahn wrote:
> efriedma wrote:
> > I don't understand what OriginalVal is when it isn't equal to CopyOf.  The example in the commit message isn't helpful here; it looks like the predicate info refers to the same value as the ssa copy.
> We need 1 more renaming step to run into the issue:
> 
> ```
> entry:
>   %lt = icmp ult i32 %a, 100
>   %a.0 = call i32 @llvm.ssa.copy.140247425954880(i32 %a) ; OriginalOp = %a
>   br i1 %lt, label %true.1, label %false
> 
> true: 
>    %gt = icmp ugt i32 %a.0, 20
>    %a.1 = call i32 @llvm.ssa.copy.140247425954880(i32 %a.0) ; OriginalOp = %a
>    br i1 %gt, label %true.2, %label %false
> ```
> 
> The OriginalOp for the info attached to %a.1 refers to %a as OriginalOp, but the actual use in %gt has been renamed due to the first copy. For most cases, using the operand is fine (as we did before), but for ANDs the operand of the second copy will be the first copy, which is not part of the Cmp instruction.
> 
> Ideally we would have OriginalOp (or another member) refer to the name of the value in the condition. Otherwise we have to walk backwards here and try to find the right value to use. I tried just checking if we have 2 chained ssa_copies, but there were some cases where this failed. I will take a closer look to check whether this should be handled here or directly in PredicateInfo. It only means that we miss out on some cases, but are able to still cover more cases as before.
> 
> (I also realized that I copied a wrong version of the example in the description. I will update it shortly.
I'm afraid this sort of "defensive" check is hiding some real bug.  I'd rather try to figure out some more consistent way to figure out the necessary information.  If we're still missing some cases, that indicates to me the algorithm for converting the information in PredicateBranches into a ConstantRange is still shaky.  And it's not obvious to me that the shakiness only manifests in the form of a missed optimization.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77808