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

[Petar Avramovic via Phabricator via llvm-commits]

Petar.Avramovic added a comment.

> I think it’s okay not to combine legal artifacts

Often they get combined before even asking if they are legal or not, there is no strict agreement on what should be combined or not it's kind of random and depends on the order we attempt combines.

> Therefore, I don’t think adding more combines would improve the legalizer. We should probably work on the current legalization strategy first before doing something like that. What do you think?

I see two routes atm, one is this patch and no more combines, other is D65894 <https://reviews.llvm.org/D65894> as it deals with combining non-legal artifacts, and to catch combines of legal artifacts we could add more combines, like D62338 <https://reviews.llvm.org/D62338> and for example add combine for G_MERGE, similar to G_TRUNC, by inspecting uses of G_MERGE def operands in hope to find G_UNMERGE and perform combine from that G_UNMERGE. With such combines it is possible to visit def before all it uses have been visited and tracking of dead instructions becomes a little difficult. It all comes down to how we menage Observer lists that hold instructions/artifacts. 
If we want to handle everything I think it is best to catch Artifact that failed to combine and retry or legalize under some conditions, the conditions when to retry combine or legalize depend on set of available combines(and maybe it would have to be reduced to set of more basic combines).

> In the mean time, I would like to commit D65894 <https://reviews.llvm.org/D65894> if you don’t have any concerns about it. It will allow us to legalize the instructions blocked by illegal artifacts without trying to improve the generated code.

Sure, let's go with D65894 <https://reviews.llvm.org/D65894> for start.


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

https://reviews.llvm.org/D61787