This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Fix legality checks for G_UBFX combines
ClosedPublic

Authored by foad on Jan 7 2022, 4:07 AM.

Details

Summary
  1. Fix CombinerHelper::matchBitfieldExtractFromAnd to check legality with the correct types for the G_UBFX that it builds.
  2. Fix AMDGPUTargetLowering::isConstantUnsignedBitfieldExtractLegal to match the legality rules: result and first operand can be s32 or s64 but the "shift amount" operands are always s32.
  3. Add AMDGPU tests where the post-legalizer combiner would create illegal MIR without the above fixes.

Diff Detail

Event Timeline

foad created this revision.Jan 7 2022, 4:07 AM
foad requested review of this revision.Jan 7 2022, 4:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 7 2022, 4:07 AM
foad added a comment.Jan 7 2022, 4:11 AM

Further cleanups are possible:

  1. Change CombinerHelper::matchBitfieldExtractFromShrAnd to use getPreferredShiftAmountTy for the shift-amount-like operands of G_UBFX, like all the other G_*BFX combines do. Change AArch64's G_*BFX legality rules to match.
  2. Remove isConstantUnsignedBitfieldExtractLegal since it doesn't seem to do any more than a standard legality check.
bcahoon accepted this revision.Jan 7 2022, 12:27 PM

Looks good to me. Thanks!

This revision is now accepted and ready to land.Jan 7 2022, 12:27 PM
This revision was landed with ongoing or failed builds.Jan 8 2022, 1:32 AM
This revision was automatically updated to reflect the committed changes.