Page MenuHomePhabricator

[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

Unit TestsFailed

TimeTest
6,200 msx64 debian > libarcher.races::task-dependency.c
Script: -- : 'RUN: at line 13'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests -I /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/src -L /var/lib/buildkite-agent/builds/llvm-project/build/lib -Wl,-rpath,/var/lib/buildkite-agent/builds/llvm-project/build/lib /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/races/task-dependency.c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-dependency.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/deflake.bash /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-dependency.c.tmp 2>&1 | tee /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/task-dependency.c.tmp.log | /var/lib/buildkite-agent/builds/llvm-project/build/./bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/races/task-dependency.c

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.EditedWed, Dec 1, 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.Thu, Dec 2, 5:27 AM

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

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