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

[Aditya Nandakumar via Phabricator via llvm-commits]

aditya_nandakumar added a comment.

In https://reviews.llvm.org/D51362#1218504, @rtereshin wrote:

> In https://reviews.llvm.org/D51362#1218451, @rtereshin wrote:
>
> > In https://reviews.llvm.org/D51362#1218394, @aemerson wrote:
> >
> > > I'm not against a separate IR canonicalization stage, even though it's a bit overkill for this particular case. At the moment it looks like InstCombine is doing the canonicalizaton for this case, so re-running that is out of the question. A new pass looks to be on the cards. Anyone else have opinions on this?
> >
> >
> > Does that mean that something after the last InstCombine produces icmp's with the non-canonical operand order? If so maybe that something needs to be chased down and fixed instead.
>
>
> I like the idea of putting this into MIRBuilder then.
>
> If IRTranslator itself uses canonicalizing MIRBuilder, it will translate IR into canonicalized MIR seamlessly w/ no evident increase in IRTranslator's complexity as it's all encapsulated within the builder.
>
> Then using the same builder everywhere else, including combines, will make sure that the canonical form is maintained across GlobalISel pipeline. For instance, one combine doesn't need to explicitly worry about not breaking the form, relying on the MIRBuilder, while the very next one can rely on MIR being in canonical form.


I guess first we'll need to define what exactly canonicalization means for this - are we talking beyond just putting constants at the end?


Repository:
  rL LLVM

https://reviews.llvm.org/D51362