[PATCH] D58096: [LowerSwitch][AMDGPU] Do not handle impossible values

[Roman Tereshin via Phabricator via llvm-commits]

rtereshin added a comment.

In D58096#1394620 <https://reviews.llvm.org/D58096#1394620>, @arsenm wrote:

> I'm not sure I see why LowerSwitch needs to worry about this optimization. Why doesn't SimplifyCFG or DCE or one of some other control flow optimization pass handle this so LowerSwitch doesn't have to worry about it?


Hi Matt,

Thank you for looking into this!

You're right, it is possible to achieve similar results with other passes, and I didn't investigate that beforehand much. Now I did and this is what I found:

1. The only passes that can help are exactly the ones that use LVI: JumpThreading and CorrelatedValuePropagation (if followed by SimplifyCFG to clean up)
2. We (a downstream GPU target) can't really do JumpThreading: it does too much and shapes CFG in ways that go against our performance targets
3. We are (re)considering doing CorrelatedValuePropagation at the moment, but:

3.1) It doesn't do much on top of eliminating dead BBs originated from lowering switches when tested on large suite of real-world shaders (there are some nice selects' eliminations here and there, but very rarely)
3.2) it consumes about 30 times more compile time than this patch using LVI within the LowerSwitch itself costs us on top of TOT LowerSwitch, which changes the extra cost from negligible to requiring consideration
3.3) In fact, it misses some of the cases and it does a poorer job eliminating dead BBs originated from LowerSwitch than this patch even in its current state (and I continue digging, looking at refining the value constraints analysis with `computeKnownBits` for instance, which can punch holes in the middle of the range with known trailing zeros)

Please let me know if you think this makes sense and if you'd like to see the LVI usage guarded by LowerSwitch's constructor variables & command line options, and if so, what's the acceptable default for it in your opinion.

I will address your other comments a bit later and update the patch, potentially making even more effort figuring out the constraints on the value.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D58096