This is an archive of the discontinued LLVM Phabricator instance.

[docs][TableGen][Target] Improve the documentation of the attribute value for SubtargetFeature.
ClosedPublic

Authored by craig.topper on Jun 16 2023, 4:13 PM.

Details

Summary

The value "true" and "false" are treated specially and other values are treated as integers.

Diff Detail

Event Timeline

craig.topper created this revision.Jun 16 2023, 4:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2023, 4:13 PM
craig.topper requested review of this revision.Jun 16 2023, 4:13 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2023, 4:13 PM
Herald added a subscriber: wdng. · View Herald Transcript

Clarify that the integer value may be the name of an enum constant.

Thanks so much for adding this! This is definitely LGTM from me, but I'll let your other reviewers decide if anything else should be added.

llvm/docs/WritingAnLLVMBackend.rst
1768

Do you think it would help to reference which parameter you're referring to in the example code?

Also, do you think it bears mentioning how the boolean value is determined? It is certainly an implementation detail, but it may be good to specify.

This sounds better behaved than

llvm/docs/WritingAnLLVMBackend.rst
1772

Is this really true? Where is this implemented?

1773

Don't refer to it as an attribute, attributes are a separate thing entirely

craig.topper added inline comments.Jun 16 2023, 8:04 PM
llvm/docs/WritingAnLLVMBackend.rst
1772

Here's the X86SSELevel code from X86GenSubtargetInfo.inc as an example

if (Bits[X86::FeatureAVX] && X86SSELevel < AVX) X86SSELevel = AVX;
if (Bits[X86::FeatureAVX2] && X86SSELevel < AVX2) X86SSELevel = AVX2;
if (Bits[X86::FeatureAVX512] && X86SSELevel < AVX512) X86SSELevel = AVX512;
1773

I was following the wording from the paragraph above and the .td. Should I change them all?

arsenm added inline comments.Jun 19 2023, 5:58 AM
llvm/docs/WritingAnLLVMBackend.rst
1773

Yes. I think there's frequent confusion between "attributes" and subtarget features in "target-features"

craig.topper added inline comments.Jun 20 2023, 6:03 PM
llvm/docs/WritingAnLLVMBackend.rst
1773

Can I do it as a follow up? Looks like I need to rename the field in the SubtargetFeature class itself to do it correctly which will involve tablegen emitter changes.

arsenm accepted this revision.Jun 20 2023, 6:07 PM
This revision is now accepted and ready to land.Jun 20 2023, 6:07 PM