This is an archive of the discontinued LLVM Phabricator instance.

[AArch64][NFC] Fix ComplexPattern types conflicting with uses
ClosedPublic

Authored by jrtc27 on Aug 26 2021, 4:58 AM.

Details

Summary

When used as a non-leaf node, TableGen does not currently use the type
of a ComplexPattern for type inference, which also means it does not
check it doesn't conflict with the use. This differs from when used as a
leaf value, where the type is used for inference. Fixing that
discrepancy is something I intend to upstream as a subsequent review,
but these are all the type conflicts found (all legitimate) by my
locally-patched TableGen.

Diff Detail

Event Timeline

jrtc27 created this revision.Aug 26 2021, 4:58 AM
jrtc27 requested review of this revision.Aug 26 2021, 4:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2021, 4:58 AM
paulwalker-arm accepted this revision.Aug 27 2021, 2:57 AM
This revision is now accepted and ready to land.Aug 27 2021, 2:57 AM

What are your plans for this and D109034 @jrtc27 ? I'm only asking because they're nice fixes that would be good to land before the next release branch.

Funnily enough I was just revisiting this last night. The AMDGPU review stalled for ages by which point AArch64 had moved on and the patches no longer worked as-is. I have something that compiles and passes all tests again but need to compare TableGen .inc files before and after again to make sure no subtle changes have crept in. So I hope to have an updated version of this review later today.

jrtc27 updated this revision to Diff 391188.EditedDec 1 2021, 7:25 PM

Rebased after patch series held up on another review for several months

Changes:

  • Trivial merge conflict resolution due to sve_ext_imm_0_* being renamed
  • Split sve_cnt_mul_imm into i32 and i64 versions due to being used as both
    • NB: The i32-using isel pattern (which is the one that gave type conflicts after rebasing, since previously the complex pattern was only ever used as an i64 so I just changed it to that, like sve_cnt_shl_imm still is) doesn't appear to have test coverage, as commenting out the new isel pattern entirely passed all llvm/lib/CodeGen/AArch64 tests
paulwalker-arm accepted this revision.Dec 2 2021, 5:27 AM

Thanks @jrtc27. Once the patch lands I'll see about fixing the missing test coverage.

Matt added a subscriber: Matt.Dec 2 2021, 9:35 AM
This revision was landed with ongoing or failed builds.Dec 2 2021, 11:05 PM
This revision was automatically updated to reflect the committed changes.

D115512 adds the missing tests.