[PATCH] D51362: [GlobalISel][IRTranslator] Canonicalize G_ICMP to have constant operands last

[Amara Emerson via Phabricator via llvm-commits]

aemerson added a comment.

In https://reviews.llvm.org/D51362#1216865, @qcolombet wrote:

> Hi Amara,
>
> That patch worries me, because I feel that we are going to pull all the LLVM IR code that does canonicalization.
>  The way I see it, is the input IR should already be in a canonical form and thus we don't have to do that.
>
> If that's not the case, I would argue that bad output code is fine (garbage in, garbage out).
>
> What do you think?
>
> Cheers,
> -Quentin


GISel is currently causing some significant code size regressions, and while this isn't the biggest issue, it looked like something that one would expect even -O0 to do. In fastisel we don't select immediate forms, however if fast-isel falls back to SDISel then the comparison between GISel enabled/disabled starts to look much worse, as SDISel selects immediates, resulting in less code and register pressure.

If we don't do this here, then I'll extend the manual G_ICMP AArch64 instruction selector code because it's extremely cheap/free. I'm ok with that at -O0, I just think that other targets will end up doing the same thing so this would be an overall code/complexity saver if we canonicalized earlier. Other optimizations will be on the way even at -O0 (like jump table creation), because being 30% larger in code size on some CTMark benchmarks is not an acceptable state at the moment.


Repository:
  rL LLVM

https://reviews.llvm.org/D51362