This is an archive of the discontinued LLVM Phabricator instance.

GlobalISel: Add bitcast to tryFoldUnmergeCast
Needs RevisionPublic

Authored by Petar.Avramovic on Jan 14 2022, 8:31 AM.

Details

Summary

Look through bitcast when unmerge searches for merge-like-opcode.
Handle the case when elements have same type.

Diff Detail

Event Timeline

Petar.Avramovic requested review of this revision.Jan 14 2022, 8:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2022, 8:31 AM
arsenm added inline comments.Feb 21 2022, 6:29 PM
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
377

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.

llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
377

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.

Herald added a project: Restricted Project. · View Herald TranscriptSep 29 2022, 8:00 AM
Herald added a subscriber: kosarev. · View Herald Transcript

Why does this have to be in the artifact combiner to work? Could we recognize this pattern post or pre-legalize? I don't have a strong opinion either way.

Bitcast can be created during legalizer. About post legalizer combiner, I don't think we run artifact combiner there, If we leave it it might block some combines combines that would become available after bitcast is combined.

Bitcast can be created during legalizer. About post legalizer combiner, I don't think we run artifact combiner there, If we leave it it might block some combines combines that would become available after bitcast is combined.

It’s not run and the artifact combiner is intended to be the minimum required for correctness

I'm not sure what to do about this. It feels right to treat bitcast as a legalization artifact, but I also can't convince myself it really is. You can always implement bitcast as a stack store and reload, so the fundamental property the artifact combiner is needed for isn't there.

arsenm requested changes to this revision.Sep 29 2022, 12:49 PM

From the test changes it seems this isn't avoiding any legalization failures, and is strictly an optimization benefit. In the future it might be worthwhile to revisit this. I have a suspicion handling bitcast here would improve compile time, but isn't strictly necessary

This revision now requires changes to proceed.Sep 29 2022, 12:49 PM