[PATCH] D40074: [GISel] Canonicalize constants to RHS for commutative operations

[Diana Picus via Phabricator via llvm-commits]

rovka added a comment.

In https://reviews.llvm.org/D40074#926470, @grandinj wrote:

> In https://reviews.llvm.org/D40074#926085, @kristof.beyls wrote:
>
> > If this is the first canonicalization (of potentially many to come) in the IRTranslator, would it make sense to try and document the canonical representation(s) somewhere and/or somehow?
>
>
> Even better would be a pass that verified canonical order, that could be inserted between other passes in debug mode to verify that optimisations preserve canonical-ness


I agree, documentation is good but it's far too easy to let it slip out of date. Would the MachineVerifier be a good place for this kind of check? 
For the record, I'm not sure this is the first canonicalization in the IRTranslator, for instance the lowering of -0.0 - X to G_FNEG instead of just another G_FSUB could be seen as a canonicalization too.

In https://reviews.llvm.org/D40074#926496, @dsanders wrote:

> I see why ARM is different from AArch64. ARM only sees one pattern being emitted because TableGen has some code that prevents commutativity being considered when one of the children of a commutative operator is either the `imm` operator or a plain integer (see `OnlyOnRHSOfCommutative()` which is used in `canPatternMatch()`).  Meanwhile, AArch64's case is using a ComplexPattern so both cases get emitted.
>
> So it appears that TableGen considers commutative pattern but chooses to ignore the second pattern because it knows theres some canonicalization taking place.


Yes, the canonicalization takes place in the target-independent part of the SelectionDAG code (SDValue SelectionDAG::getNode(unsigned Opcode, const SDLoc &DL, EVT VT, SDValue N1, SDValue N2, const SDNodeFlags Flags).

I guess one advantage that DAGISel has over GlobalISel is that the getNode helpers are used throughout instruction selection. If we wanted the same kind of behaviour for GlobalISel, we'd have to put this canonicalization in the MachineInstrBuilder, but that would affect the whole backend.

I think for now it's better to keep this in the IRTranslator since it's the least disruptive thing to do, and any subsequent passes (e.g. combiners and whatnot) can benefit from having a canonical form to work with most of the time. An alternative would be to move these things into their own canonicalization pass that we can then schedule whenever we see fit, but that might be a bit overkill since adding canonicalizations isn't really a priority at this point.


https://reviews.llvm.org/D40074