[PATCH] D74484: [AggressiveInstCombine] Add support for ICmp instr that feeds a select intsr's condition operand.

[Ayman Musa via Phabricator via llvm-commits]

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


================
Comment at: llvm/test/Transforms/AggressiveInstCombine/trunc_select_cmp.ll:4-5
+
+; Today, InstCombine cannot handle the following cases since it doesn't
+; allow - in any way - an instruction with multiple uses to be shrunk.
+; Aggressive InstCombine, on the other hand, remembers all the nodes we
----------------
spatel wrote:
> This isn't entirely accurate for the tests as shown. The problem in several of these tests is not that there are extra uses; it's that we don't canonicalize icmp and min/max as well as possible. That was what rGf2fbdf76d8d0 was trying to solve, but I don't think there's a quick fix to restore that.
> 
> Please add some extra uses in these first 4 tests, so we have a better representation of the motivating tests. One way to do that is to add something like:
> declare void @use(i32)
> and then:
> call void @use(i32 %conv)
I must disagree with you this time.
The infrastructure for shrinking such cases inside InstCombine cannot handle these cases.
If we look, for example, at the first case in this test, notice that the "sext" instruction has multiple uses.
Which will prevent the mechanism inside instcombine from shrinking the whole sequence, even though both uses are going to be shrunk together (as part of the same sequence) with "sext".
This is one of the main additions in AggressiveInstCombine, where while we're checking the validity/possibility of shrinking sequences, we remember all the instructions we encountered until now. That enables us to check if all the uses of such instruction are included in these instructions to make sure we'll be replacing **all** the uses of the old sext instruction with the new one.


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

https://reviews.llvm.org/D74484