[PATCH] D117328: GlobalISel: Add bitcast to tryFoldUnmergeCast

Tue Feb 22 02:54:32 PST 2022

Petar.Avramovic added inline comments.

================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h:373

-    if (!isArtifactCast(CastOpc))
+    if (!isArtifactCast(CastOpc) && CastOpc != TargetOpcode::G_BITCAST)
       return false;
----------------
arsenm wrote:
> It feels inappropriate for the artifact combiner to operate on something that isn't defined as a legalization artifact. We currently don't call G_BITCAST a legalization artifact, but is there any real reason for that? Should we just start treating it as one? It gets murky because we currently lower G_BITCAST into merge+unmerge, so that sort of implies it's not fundamentally an artifact.
I don't have strong opinion if G_BITCAST should be an artifact because artifact combiner mentions it. Also tryFoldUnmergeCast's name suggests that it folds casts and in this change we look through cast (G_BITCAST).
Only (bad/slowdown) change would be that legalizer would add bitcast instructions to ArtifactList but no combine starts from G_BITCAST and they would end up in InstList anyway (failing combine that couldn't happen).
Do we have formal definition of what is an artifact? I found: 
>Legalization Artifacts are all those insts that are there to make the type system happy. 
Only effect of being in isArtifact, that I am aware of, is the need for instruction to be in ArtifactList.

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

https://reviews.llvm.org/D117328