This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Add G_BUILD_VECTOR[_TRUNC] to CSE
ClosedPublic

Authored by rovka on Jan 4 2023, 2:40 AM.

Details

Summary

Add G_BUILD_VECTOR and G_BUILD_VECTOR_TRUNC to the list of opcodes in
shouldCSEOpc. This simplifies the code generated for vector splats.

Diff Detail

Event Timeline

rovka created this revision.Jan 4 2023, 2:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 4 2023, 2:40 AM
rovka requested review of this revision.Jan 4 2023, 2:40 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 4 2023, 2:40 AM
foad accepted this revision.Jan 4 2023, 3:39 AM
foad added a subscriber: foad.

LGTM!

llvm/test/CodeGen/AArch64/GlobalISel/prelegalizer-combiner-mulo-zero.mir
91

What happened here?

llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-build-vector-splat.mir
1

Maybe precommit this test so we can see the diff?

This revision is now accepted and ready to land.Jan 4 2023, 3:39 AM
arsenm accepted this revision.Jan 4 2023, 11:01 AM
rovka added inline comments.Jan 5 2023, 1:29 AM
llvm/test/CodeGen/AArch64/GlobalISel/prelegalizer-combiner-mulo-zero.mir
91

Well, my hypothesis is that:
Without CSE for G_BUILD_VECTOR, we replace the G_UMULO with a brand new G_BUILD_VECTOR, thus leaving the already existing G_BUILD_VECTOR (%zero_vec) without a use, so it just gets removed.
With CSE enabled, when we try to build the new G_BUILD_VECTOR for our combine, we notice we already have one and just copy it, so now it has a use and can't be dropped. I'm not sure why we don't notice that we can use it directly instead of copying though.
I could dig deeper into this, unless we have some code elsewhere in the pipeline that gets rid of redundant copies?

llvm/test/CodeGen/AMDGPU/GlobalISel/legalize-build-vector-splat.mir
1
rovka updated this revision to Diff 486496.Jan 5 2023, 1:30 AM

Rebase.

This revision was landed with ongoing or failed builds.Jan 5 2023, 1:31 AM
This revision was automatically updated to reflect the committed changes.
foad added inline comments.Jan 5 2023, 2:09 AM
llvm/test/CodeGen/AArch64/GlobalISel/prelegalizer-combiner-mulo-zero.mir
91

There is a "copy_prop" combine that should do that, and AArch64PreLegalizerCombiner runs "all_combines", so I don't understand why it's not happening.

rovka added inline comments.Jan 5 2023, 3:21 AM
llvm/test/CodeGen/AArch64/GlobalISel/prelegalizer-combiner-mulo-zero.mir
91

Oh, that's easy then, the test is run with --aarch64prelegalizercombinerhelper-only-enable-rule="mulo_by_0" :)

foad added inline comments.Jan 5 2023, 5:20 AM
llvm/test/CodeGen/AArch64/GlobalISel/prelegalizer-combiner-mulo-zero.mir
91

Thanks! I did not even know you could do that.