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

[Volkan Keles via Phabricator via llvm-commits]

volkan added a comment.

In D61787#1620875 <https://reviews.llvm.org/D61787#1620875>, @Petar.Avramovic wrote:

> @volkan Regarding D65894 <https://reviews.llvm.org/D65894>.
>  Here, artifacts that failed to combine are moved to RetryList and we retry to combine them. Once all of the MachineInstrs that define use operands of our artifact are processed (are not in any of the Observer Lists) we turn artifact into an instruction. This way we have more opportunities for combines. e.g. in `/test/AMDGPU/GlobalISel/legalize-unmerge-values.mir` , this patch catches G_MERGE/G_UNMERGE combine that D65894 <https://reviews.llvm.org/D65894> cannot catch because those two are declared legal.
>  As for the other test changes in D65894 <https://reviews.llvm.org/D65894>, all of them are essentially here.
>  I also tried `test/CodeGen/AArch64/GlobalISel/retry-artifact-combine.mir` and got similar output, but with one less copy instr.
>  there are differences in `test/CodeGen/AMDGPU/GlobalISel/legalize-{xor|and|or}.mir`
>  D65894 <https://reviews.llvm.org/D65894> managed to combine few more G_TRUNC + G_ANYEXT but in the end both patches crash on  `%0:_(<4 x s8>) = G_TRUNC %29:_(<4 x s32>),` because order of combine attempts is different this patch crashed before attempting to combine mentioned G_TRUNC + G_ANYEXT.
>  Please check if I missed something the this patch does not cover compared to D65894 <https://reviews.llvm.org/D65894>.
>
> See D65199 <https://reviews.llvm.org/D65199> for more tests with output from -debug option.


Hi Petar,

I completely forgot this patch exists, sorry about it. I took a look and found a few issues.

First of all, I don't see a reason to add another concept called AuxilaryArtifacts, it makes the legalization really complicated. I think we can achieve the same result with the current design. Here is an example:

  ...
  %1:_(s8) = G_TRUNC %0(s32)
  ...
  %9:_(s32) = G_ANYEXT %1(s8)

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?

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.

  ...
  %9:_(s64) = G_ANYEXT %0(s32)
  ...
  %1:_(s32) = G_TRUNC %9(s64)
  ...

Regarding the algorithm, I'm not sure if it's always a good idea to wait for the source instructions before legalizing artifacts, that why I didn't want to change that in D65894 <https://reviews.llvm.org/D65894>. 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. The other reason is that you keep processing the artifact that are legal for no reason. 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?

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?


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

https://reviews.llvm.org/D61787