[PATCH] D78391: [ValueLattice] Allow two range extensions.

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/include/llvm/Analysis/ValueLattice.h:358
       // otherwise.
-      if (Opts.CheckWiden && ++NumRangeExtensions >= 1) {
+      if (Opts.CheckWiden && ++NumRangeExtensions >= 2) {
         // Make sure WidenCR is only used if it contains the merge result.
----------------
Add `MaxRangeExtensions = 2` static const?


================
Comment at: llvm/test/Transforms/SCCP/constant-range-struct.ll:106
+; CHECK-NEXT:    call void @use(i1 true)
+; CHECK-NEXT:    call void @use(i1 true)
 ; CHECK-NEXT:    [[T_3:%.*]] = icmp ne i64 [[V2]], 0
----------------
Disclaimer: First time I'm looking at SCCP in LLVM.

I feel like we really should be handling this already, without support for multiple widenings (and also, we should handle this for cases with more phi arguments). What happens in `@struct2` is that we first visit the entry block, then the `false` block, then the `exit` block (computing everything there), then the `true` block, then `true -> exit` becomes feasible and we update the phi constant ranges, and then merge in new values for all instructions in `exit`, going to overdefined (without widening).

If we had instead visited `entry`, `false`, `true`, `exit`, then not only would we have saved on duplicate computations, we would have also avoided the need to perform widening.

I feel like the right way to approach this is to fix the block visitation order, and then more widening is something we can do on top of that (which should then only matter for loops).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78391