[PATCH] D45862: [InstCombine] Relax restriction in foldSelectInstWithICmp for sake of smaller code size

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added a reviewer: bkramer.
mkazantsev added a comment.

Hi @spatel @lebedev.ri,

Actually I am absolutely agreed that these transforms produce really bad code which looks like a pessimization breach. Let me give you a bit more context about my motivation for this change. I basically need that the following pattern was recognized correctly as three-way comparison:

  %cmp1 = icmp eq %x, 0
  %cmp2 = icmp slt %x, 0
  %s1 = select i1 %cmp2, -1, 1
  %s2 = select i1 %cmp1, 0, %s1

It is a typical pattern that we see in all comparators. We have a function `matchThreeWayIntCompare` that used to detect such a pattern. However at some point InstCombine started recognizing the pattern of `%cmp2`, `%s1` as a bit test of the highest bit, removed it and inserted the shift-based bit test instead. Even worse, it does insert *different* patterns for case when both `%x` and `%s1` are `i32` or when they are of different types.

The problem is wide-spread across InstCombine: we seem to have *many* transforms of this kind spread across the code. And which of them will be applied is always a surprise: some of them are implemented for general case and some are limited. As result, we might end up with a dozen of different bit-test patterns that all represent a three-ways comparison.

This patch was an attempt to canonicalize all these transforms, so that at least for the same pattern they were done in the same manner regardless of types. Here I was taking an assumption like "okay, we are doing all this bit-test stuff anyways, let's at least do it uniformly". Because now what we have is a counter-canonicalization of this pattern.

I agree that in ideal world we should just remove all this job connected to adding bit-tests instead of this select, or even do it in opposite way (so that bit-tests are converted to select). I see two reasons why it is impossible.

1. We have no idea how it impacts performance in the particular cases for which they were added.
2. We have little understanding of how other transforms and optimizations will react on that, and will we break something or not. I have a crawling suspicion that all this stuff was done for vectorization, and that the vectorizer might expect this bit stuff rather than selects. I need someone familiar with vectorizer's code to confirm or refute that.

I will be totally happy if we decide to not transform selects into bittests. But I am hesitant to make a change this big in the part of code I am unfamiliar with, as well as I am unfamiliar with the reasons why it was done at all and whether it helps the vectorization. So I need the confirmation from vectorizer people that selects there are OK.

For now, my point is the following: yes, we are *already* doing all this bit magic. Yes, it looks like miscanonicalization, and we can discuss getting rid of all such transforms at all. But since we are already doing it and haven't yet decided to clear it off, let's at least do it well. Once we decide that all this stuff is counter-canonical, we can happily remove all this. What do you think of that?

Thanks,
Max

P.S. I've also added Benjamin Kramer who was the original author of this transform to take a look.


https://reviews.llvm.org/D45862