This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Fix invalid combine of unmerge(merge) with intermediate cast
ClosedPublic

Authored by gargaroff on Apr 15 2020, 3:30 AM.

Details

Summary

The combine for unmerge(cast(merge)) is only valid for vectors, but was
missing a corresponding check. Add a check that the operands are vectors
to avoid an invalid combine.

Without this check, the combiner would emit incorrect code for scalars
and pointers because the artifact cast (trunc/ext) only affects bits at
the end of the type, while this combine assumes that the casted bits
appear between meaningful bits.

This also uncovered a segmentation fault in the AMDGPU
InstructionSelector. The tests triggering this bug have been moved to
their own file and a check for the segmentation fault has been added.

Diff Detail

Event Timeline

gargaroff created this revision.Apr 15 2020, 3:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2020, 3:30 AM
gargaroff edited the summary of this revision. (Show Details)Apr 15 2020, 3:36 AM
arsenm added inline comments.Apr 15 2020, 5:46 AM
llvm/include/llvm/CodeGen/GlobalISel/LegalizationArtifactCombiner.h
528–530

canFoldMergeOpcode should have done this legality check

llvm/test/CodeGen/AMDGPU/GlobalISel/artifact-combiner-unmerge-values.mir
915

It doesn't really matter, but this should use different source registers for the two copies. The second should be $vgpr2_vgpr3

938

$vgpr1

llvm/test/CodeGen/AMDGPU/GlobalISel/cvt_f32_ubyte-xfail.ll
4

This will only work on some linuxes sometimes, there’s not much point in checking the error though

gargaroff updated this revision to Diff 257713.Apr 15 2020, 7:17 AM
gargaroff marked 4 inline comments as done.

move legality check to canFoldMergeOpcode

use difference source and destination registers in tests

remove error check from xfail test, only expect crash

arsenm accepted this revision.Apr 15 2020, 7:39 AM
This revision is now accepted and ready to land.Apr 15 2020, 7:39 AM
This revision was automatically updated to reflect the committed changes.
gargaroff reopened this revision.Apr 15 2020, 9:51 AM

Unfortunately this caused the build bots to fail due to the sanitizers flagging undefined behavior in the AMDGPUInstructionSelector and I have reverted the commit for now. @arsenm Do you think this patch should wait until the bug in the InstructionSelector is fixed? Or should this patch include a fix? I have not looked into the problem yet, so I cannot say what goes wrong exactly.

This revision is now accepted and ready to land.Apr 15 2020, 9:51 AM

Unfortunately this caused the build bots to fail due to the sanitizers flagging undefined behavior in the AMDGPUInstructionSelector and I have reverted the commit for now. @arsenm Do you think this patch should wait until the bug in the InstructionSelector is fixed? Or should this patch include a fix? I have not looked into the problem yet, so I cannot say what goes wrong exactly.

I'm assuming this is hitting a null dereference due to the missing 192-bit register class

Unfortunately this caused the build bots to fail due to the sanitizers flagging undefined behavior in the AMDGPUInstructionSelector and I have reverted the commit for now. @arsenm Do you think this patch should wait until the bug in the InstructionSelector is fixed? Or should this patch include a fix? I have not looked into the problem yet, so I cannot say what goes wrong exactly.

I'm assuming this is hitting a null dereference due to the missing 192-bit register class

Try after 588bd7be366620d2319d349f7665b503d7840f45