[PATCH] D103660: [ScalarEvolution] Fix pointer/int type handling converting select/phi to min/max.

Wed Jun 16 08:36:57 PDT 2021

reames added a comment.

The revised change looks a lot more reasonable.  With some cleanup, I'd be willing to LGTM this.  I'm much happier with the explicit focus on avoiding the construction of a subtract of pointer type.

You combined the logic for the signed and unsigned case.  Can I ask that you commit an NFC change which does that, and then rebase this?  The overall structure is fine for the NFC version, I just want the diffs to stand out in this change.

Your suggested rules in the previous comment also seem to be a reasonable direction.  My concern is around the subtract case.  As you've noted, there's a bunch of cases where we subtract pointers today, and finding variants for those is going to taken some work.  I'd also *strongly* encourage you to encode your rules as assertions.  :)

Hm, have you considered doing the coercion check inside getMinusSCEV?  If the construct we're trying to outlaw is a subtract of pointers, maybe we should just explicitly do that?  (I'm fine with a cleaned up version of this landing, then exploring that if desired.)

================
Comment at: llvm/lib/Analysis/ScalarEvolution.cpp:5554
+        // negated pointers.
+        const SCEV *LS = getSCEV(LHS);
+        const SCEV *RS = getSCEV(RHS);
----------------
Repeated code, masking a variable.

================
Comment at: llvm/test/Transforms/IndVarSimplify/pr45835.ll:13
 entry:
-  %cmp = icmp ule i8* %c, getelementptr inbounds (i8, i8* @a, i64 65535)
+  %cmp = icmp ule i8* %c, @a
   %add.ptr = getelementptr inbounds i8, i8* %c, i64 -65535
----------------
Can you explain this test change?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103660