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

Fri Aug 9 15:40:01 PDT 2019

Petar.Avramovic added a comment.

> I don't think that matters as long as you process the rest of the instructions before aborting. That's what D65894 <https://reviews.llvm.org/D65894> does, it keeps processing the instructions in InstList, then tries to combine the illegal artifacts if there is anything new. Do you have a test case that requires more complicated logic?

Yes, D65894 <https://reviews.llvm.org/D65894> works perfectly fine for non legal artifacts, but it can't deal with legal artifacts that could be combined.
File withD61787.txt in D65199 <https://reviews.llvm.org/D65199> 
line 920 in second file, Inspecting Artifact : %10:_(s32), %11:_(s32) = G_UNMERGE_VALUES %6:_(s64)

D65894 <https://reviews.llvm.org/D65894> would try to legalize that G_UNMERGE and it is legal, that is it, it is not in any observer list any more, we can't combine it.

If we add tryCombine for G_MERGE_VALUES similar to what G_TRUNC does we might be able to combine them, but again that means to combine instruction that is not in any observer List and is legal.

or D61289 <https://reviews.llvm.org/D61289>, see my comment from May 22 2019

> Let's assume {G_SEXT, DstTy, SrcTy} is marked as Custom. In this case, if you skip legalization and try to combine them aggressively, the ArtifactCombiner will generate G_SHL and G_ASHR, but it doesn't check if this is something that the target wants. As I said before, currently, we don't have any control over these combines.

Oh that, I agree that targets should get some control over how to combine some artifacts and just as we speak about it D61289 <https://reviews.llvm.org/D61289> just landed so target gets control over how G_TRUNC+G_SEXT are combined by legalizing G_SEXT_INREG. However I don't thik that is the main focus here as I tried to figure out when is it justified to wait and try to combine again, and when we have to legalize. Again I only considered combines currently availabe, this patch most probably won't work if we try to add more combines (but it works if we narrow scalar artifacts).

> The problem with adding more artifact combines is that targets have no control over it, that's why I don't think it's a good idea to delay the legalization of the artifacts as much as possible in order to combine more instructions, unless it's not supported by the target.
> Yes, maybe that's what we need to do, the ArtifactCombiner can check if it's legal or not.
> 
>> Artifacts should be combined regardless if they are legal or not, e.g. s32 G_ANYEXT s8 is legal for AArch64 but almost always gets combined.
> 
> I din't think that's always a good idea, see my example above (G_SEXT).

Let's call it detect more combine opportunities instead of add more combines.
We could add hooks for target custom combine but I am not that sure we need them. 
I think that Artifact combiner should not care if something is legal or not, it should manage its oberver lists and be able to combine/notify target that combine is available, when two connected artifact appear during legalization.

> Is it necessary if they are already legal? The goal is to clean up the instruction generated by unsupported types, not to optimize generated MF. That's should be the combiner's job.

e.g. on AArch64 G_ZEXT, G_SEXT, G_ANYEXT and G_TRUNC are legal for all combinations of most common types (s8, s16, s32, s64) and you will not see them leave legalizer next to each other uncombined. On the other hand uncombined G_MERGE/G_UNMERGE can leave legalizer. I think its desireable to combine everything available, it's is a little extra effort vs. having one extra pass that does same thing but is smarter and can combine everything.

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

https://reviews.llvm.org/D61787