This is an archive of the discontinued LLVM Phabricator instance.

[ARM][AArch64] Use StringRef in TargetParser structs
ClosedPublic

Authored by tmatheson on Nov 15 2022, 4:55 AM.

Details

Summary

The invalid case is now represented by an empty StringRef rather than
a nullptr.

Previously ARCH_FEATURE was build from SUB_ARCH by prepending "+".
This is now reverse, so that the "+arch-feature" is now visible in
the .def, which is a bit clearer. This meant converting one StringSwitch
into a loop.

Removed getters which are now mostly unnecessary.

Removed some old FIXMEs.

Diff Detail

Event Timeline

tmatheson created this revision.Nov 15 2022, 4:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 15 2022, 4:55 AM
tmatheson requested review of this revision.Nov 15 2022, 4:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 15 2022, 4:55 AM
pratlucas added inline comments.Nov 15 2022, 5:30 AM
llvm/include/llvm/Support/AArch64TargetParser.def
20–21

Having the lone "+" as arch_feature here looks a bit off. Could we remove the "invalid" entries from the def file completely?
Would it make sense to use the INVALID enum entries by themselves to handle unsuccessful parsing, or maybe use optional returns on the relevant methods?

tmatheson added inline comments.Nov 15 2022, 5:32 AM
llvm/include/llvm/Support/AArch64TargetParser.def
20–21

It does indeed look weird, but that's what it is already doing internally so there is NFC. This patch is already quite large so refactoring the invalid entries would be best done in a follow up.

lenary accepted this revision.Nov 15 2022, 10:01 AM

I'm happy with this, and happy for the invalid case to be sorted in a follow-up.

This revision is now accepted and ready to land.Nov 15 2022, 10:01 AM

Rebase after 8.9/9.4

This is now reverse, so that the "+arch-feature" is now visible in the .def, which is a bit clearer.

Clearer for what exactly.

llvm/lib/Support/AArch64TargetParser.cpp
110–111

Maybe this is just me but "!empty()" could just be "size()".

I know sometimes containers make empty cheaper but here it's just == 0 vs != 0.

This is now reverse, so that the "+arch-feature" is now visible in the .def, which is a bit clearer.

Clearer for what exactly.

Previously the string literal you would see in the ArmTargetParser.def was 'xyz', which would be stored on the struct as '+xyz' (a string literal created by a macro) and on the command line you would write +xyz. Now that it is written +xyz in the .def file, the correspondence between the three is more obvious, imho. Searching for +xyz will actually lead you to where it is defined.

llvm/lib/Support/AArch64TargetParser.cpp
110–111

For checking that "the feature is not the empty string", !empty() makes a lot more sense to me semantically than an implicit cast<bool>(size()). I'm trying to move away from the C-style code :) I would hope (haven't checked) that they compile to basically the same thing.

DavidSpickett accepted this revision.Nov 17 2022, 7:44 AM

Searching for +xyz will actually lead you to where it is defined.

Sure, that makes sense (especially in the current state where the .def is the only documentation).

llvm/lib/Support/AArch64TargetParser.cpp
110–111

Agree to disagree, keep it as you prefer.

tmatheson updated this revision to Diff 476140.Nov 17 2022, 8:15 AM

Rebase for CI

tmatheson updated this revision to Diff 476160.Nov 17 2022, 9:25 AM

Update unittest TargetParserTest.cpp

tschuett added a subscriber: tschuett.EditedNov 17 2022, 9:41 AM

Optional<StringRef> is too verbose to model the invalid case?

This revision was automatically updated to reflect the committed changes.

Optional<StringRef> is too verbose to model the invalid case?

Sorry I missed this before I pushed. Optional is a potential option (heh), it wasn't done here simply because I'm trying to keep the changes incremental. I will make a follow up patch if I can find something reasonable that works.