[PATCH] D61787: [GlobalISel Legalizer] Improve artifact combiner

Thu Jul 25 04:36:04 PDT 2019

Petar.Avramovic added a comment.

> I understand that doing this may allow more artifacts to be combined away, but it shouldn't change the legality of anything coming out of the legalizer, if properly specified, right?

Absolutely. When some artifact is legal it can leave legalizer. It also needs few other instructions that are legal for same types so that it can connect them. e.g. for AArch64 G_PHI is legal for s8 and G_ADD is legal for s32 you expect  `s32 = G_ZEXT s8` to be legal and could leave legalizer since we could have sequence like this:

  %10:_(s8) = G_PHI ....
  %11:_(s32) = G_ZEXT %10:(s8)
  %12:_(s32) = G_ADD %11:(s32), %8:(s32)

This patch does not affect legal artifacts in any way (it might be able to perform few combines more then what we have atm because it checks if uncombined artifact is chained to some unprocessed instr and might try to combine again later).
 It won't ask artifact is it legal as long there is a instr in some of the lists that could create other half necessary for combine (here I named other half auxiliary artifact).

On the other hand, at the moment, smallest legal type for MIPS is s32. If s8 appears it should be dealt with widen scalar and G_ZEXT/G_SEXT should never leave legalizer.
This patch is able to perform all combines without declaring  G_ZEXT/G_SEXT as legal. Combiner as is now, can perform them (with recursive combine when G_TRUNC jumps to its uses) but needs G_ZEXT/G_SEXT to be legal in order to not crash during legalization. However combiner as is now is not able to combine all G_MERGE/G_UNMERGE.
Patch was originally made for narrow scalar of artifacts. It might be easier to think of large types  that cannot fit into register and not having to make merge/unmerge legal for them since we cannot deal with them later (efficiently at least).  
I think that we should be dealt with big types with narrow scalar and artifact combiner without having to declare artifacts legal for big types.

Comments got too long and are hard to follow here, we could start again in D65199 <https://reviews.llvm.org/D65199>.

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

https://reviews.llvm.org/D61787