[PATCH] D104355: [GlobalISel] Add a new artifact combiner for unmerge which looks through general artifact expressions.

Wed Jun 16 14:01:43 PDT 2021

aemerson added a comment.

In D104355#2822379 <https://reviews.llvm.org/D104355#2822379>, @qcolombet wrote:

> Hi Amara,
>
> I like this approach!
> What is the impact on compile time?

Compile time seems to be noise, since this the new combine only runs in some cases where the unmerge combine fails, which is rare in most code. I would have liked to use it as the first-attempt combine for unmerge, but AMDGPU just wound up getting into a legalization loop (I think that's a problem with AMDGPU rather than the combine itself).

> I am a little bit surprised by the long sequences that we generate now in certain case. What is the reason for that? (See inline comment for a highlight of what I am talking about.)
>
> Cheers,
> -Quentin

================
Comment at: llvm/test/CodeGen/AArch64/GlobalISel/legalize-inserts.mir:864
+    ; CHECK: $x0 = COPY [[MV]](s64)
+    ; CHECK: $x1 = COPY [[MV1]](s64)
     %0:_(s64) = COPY $x0
----------------
qcolombet wrote:
> Is this new sequence what we really want?
> 
> It seems complicated compared to what we have before that diff.
This looks to be the result of marking G_INSERT an artifact, which changes the order legalization vs artifact combines. It's not related to the ArtifactValueFinder, so it's collateral damage unfortunately.

In fact, all of these test changes except for the new tests I added are changes as a result of G_INSERT being an artifact.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104355