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

[Florian Hahn via Phabricator via llvm-commits]

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


================
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
----------------
nikic wrote:
> nikic wrote:
> > 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).
> Just as a quick experiment, visiting BBs in BFS instead of DFS: https://gist.github.com/nikic/85824a0d14225cd1af5a8b0e25982c0e
> 
> Some wins, some losses. What we really want is to visit the BBs in RPO order.
> 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).

In some cases it boils down to iteration order, yes. What makes things a bit more tricky is that the order of blocks also depends on the current value of the branch conditions (i.e. if we can prove a condition is true/false we don't need to visit one of the predecessors), which in turn depends on the iteration order. While only constants were supported, it did not matter too much, but becomes more relevant with constant ranges.

In the example above, we have to execute both true and false straight away, as the condition is overdefined. In other scenarios a different iteration order won't help unfortunately.

Granted, RPO would probably be better in general, but more expensive and we probably would have to keep the worklist in RPO order as well. It might be worth a try though (and not matter too much in terms of compile-time). I'll put something together, but the widening extension and iteration order address different issues (although there is some overlap)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78391