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

Fri Aug 9 08:53:01 PDT 2019

Petar.Avramovic added a comment.

> In this case, moving %1 to AuxiliaryArtifactList doesn't make any difference because combining %9 will remove it anyway, even if it was in InstList. Since you already check the source registers before legalizing the artifacts, this shouldn't be a problem. What do you think?

Well in this example it is not a problem, but in more complicated case:
moving %1 to AuxiliaryArtifactList means that %1 will be legalized first if %9 failed to combine it away, %9 might be able to combine away the artifact that was produced during legalization of %1.
In general, this patch only combines Artfact from ArtifactList with some auxiliary artifact from AuxiliaryArtifactList, if something gets legalized to legal it will not dispersal from MF during some combine. It's a bit confusing to see instruction said to be legal during legalization but it is not in the final MF.

Other option is to add much more artifact combines to make sure if two artifact are connected, they can be combined away. I guess this way we should eventually lose need for artifacts being anything other then legal. Also since there is no "order for combine attempts" and other part required for combine might be hidden behind and instruction (e.g. G_SEXT_IN_REG) we have to make all combines to jump to its use and try from there e.g G_MERGE has to try to jump to use of its def Operand since corresponding G_UNMERGE could be already legalized.

> Also, the definition of auxiliary artifact didn't seem right to me. It might be the opposite depending on the function. For example, the G_TRUNC instruction below has to wait for %9 to get combined. %9 should be auxiliary artifact in this case.

This would be true if there was an G_ANYEXT + G_TRUNC combine, but we don't have one (we have G_TRUNC + G_ANYEXT);
I am pretty sure that you can't get this sequence of instructions from llvm-ir input with what we have in the tree. There might be some narrowScalars in the future that could create such sequence but then we can narrow scalar both artifacts and continue from there.

I have only considered what we have in tree and regression tests. Artifacts are cases from switch in tryCombineInstruction(...
excluding G_TRUNC since it jumps to uses of its operand 0 and recursively calls tryCombineInstruction

Anyway since auxiliary artifacts just complicate things i tried similar thing again but instead of having concept of auxiliary artifacts, I check for both inputs and outputs(this is new) of Artifacts that failed to combine and if some of them is in any Observer list I push it to RetryList. Problem with this is that at some point we are left with chain of artifacts and only thing left to do is to try to legalize. Now if we legalize in wrong order and don't try to combine first we might miss combine since legal instruction gets out of Observer Lists and won't be combined.

> Regarding the algorithm, I'm not sure if it's always a good idea to wait for the source instructions before legalizing artifacts.

Probably not always, I pretty much extended what happens in MF where all instructions are either legal or widenScalar, Legalize instructions (sources) first, combine artifacts and we are done (at the end we just check that artifact combines generated only legal instructions)

> One reason is generated instruction might be expensive for targets. For example, in order to combine G_SEXT, we emit G_SHL and G_ASHR, but we don't check if it's ideal for the target.

I don't understand this one.

> The other reason is that you keep processing the artifact that are legal for no reason.

Yes, but algorithm doesn't know if that artifact is legal, it just waits for its input to get legalized and to retry combine. 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.

> So, I think we need a long term solution if we are going to change the legalization strategy, maybe one with a target hook to decide how to process artifacts?

I am not so sure about this one, that hook will have to hold pretty much entire body of runOnMachineFunction because do/while loop is different and observer is different.

> Lastly, does D65894 <https://reviews.llvm.org/D65894> fix your issue? If so, do you think it can be used as a short-term solution?

Hm.. It should fix the not having artifacts to be legal in order to combine them. But it misses to combine artifacts that are legal(RegBankSelect needs G_MERGE and G_UNMERGE legal and is able to deal with them). We could try adding few more combines for start and see how it works.

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

https://reviews.llvm.org/D61787