This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Inline AArch64TargetParser.def
ClosedPublic

Authored by tmatheson on Dec 1 2022, 4:53 AM.

Details

Summary

Inline calls for the following macros and delete AArch64TargetParser.def:
AARCH64_ARCH, AARCH64_CPU_NAME, AARCH64_CPU_ALIAS, AARCH64_ARCH_EXT_NAME

Diff Detail

Event Timeline

tmatheson created this revision.Dec 1 2022, 4:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2022, 4:53 AM
tmatheson requested review of this revision.Dec 1 2022, 4:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2022, 4:53 AM

A momentous occasion :) Surprised we don't have more uses of this but that's probably because you've been fixing them too.

Please update the comment in lldb/test/Shell/Commands/command-disassemble-aarch64-extensions.s as well.

I will throw a bit of a wrench in here though.

@andrewrk I seem to remember that Zig uses these files for generating compiler options of its own? A basic search didn't show anything but we'd better check if you're using them still.

Found it. This was the llvm dev thread: https://lists.llvm.org/pipermail/llvm-dev/2020-January/138548.html

And this is the file that was being generated: https://github.com/ziglang/zig/blob/c86589a738e5053a26b2be7d156bcfee1565f00b/lib/std/target/aarch64.zig

I don't see the scripting (if any) that made it though.

tmatheson updated this revision to Diff 479598.Dec 2 2022, 4:35 AM

Update lldb test

Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2022, 4:35 AM

And this is the file that was being generated: https://github.com/ziglang/zig/blob/c86589a738e5053a26b2be7d156bcfee1565f00b/lib/std/target/aarch64.zig

Thanks for the heads up. It looks like that file hasn't been updated for a couple of years, and doesn't include any recent features, so unless we hear otherwise from @andrewrk I think we can go ahead. If some script _is_ manually parsing the .def, it ought to be easy enough to update it to read the .cpp instead (it probably shouldn't do either).

Please update the comment in lldb/test/Shell/Commands/command-disassemble-aarch64-extensions.s as well.

I have reordered the list to be alphabetical and updated the comment, added tests for recent features that were missed (CSSC, D128, RCPC3, THE, and a few others) and used CHECK-NEXT.

DavidSpickett accepted this revision.Dec 5 2022, 1:30 AM

If some script _is_ manually parsing the .def, it ought to be easy enough to update it to read the .cpp instead (it probably shouldn't do either).

True. Hopefully the target parser library approach will give us a nice way to query all this stuff without manual conversion.

This LGTM with the small test change done.

lldb/test/Shell/Commands/command-disassemble-aarch64-extensions.s
28

This one always trips me up because the offset changes when I update the test.

Can you move lbl to the start, or just use fn as the label, if that works?

This revision is now accepted and ready to land.Dec 5 2022, 1:30 AM

And thanks for bringing the test up to date in general!

(that test is less checking the disassembler, more a reference of what lldb should be capable of, though it has found some bugs)

Matt added a subscriber: Matt.Dec 5 2022, 12:01 PM

@andrewrk I seem to remember that Zig uses these files for generating compiler options of its own? A basic search didn't show anything but we'd better check if you're using them still.

Thanks for thinking of me.

Zig uses the .td files directly, with this script: https://github.com/ziglang/zig/blob/0.10.0/tools/update_cpu_features.zig

So, in this case, unless you're changing the data layout of AArch64.td, our script will be unaffected. Even if you did change it though, it would be OK - I have no expectation for this data to be stable, and this script is run offline only when upgrading to a new LLVM version. So I fully expect to have to rewrite this script in the future.

llvm/include/llvm/Support/AArch64TargetParser.h