Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang] Enable descriptions for --print-supported-extensions #66715

Merged
merged 1 commit into from
Sep 22, 2023
Merged

[clang] Enable descriptions for --print-supported-extensions #66715

merged 1 commit into from
Sep 22, 2023

Conversation

cbalint13
Copy link
Contributor

@cbalint13 cbalint13 commented Sep 18, 2023

Enables summary descriptions along with the names of the feature.
Descriptions here are simply looked up via the available llvm tablegen data.

@llvmbot llvmbot added clang Clang issues not falling into any other category backend:RISC-V llvm:support labels Sep 18, 2023
Copy link
Collaborator

@DavidSpickett DavidSpickett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really nice. Two overall things:

Now that ARM and AArch64 print more than just names, they should have column headers like RISC-V does. Name / Description

The clang test case clang/test/Driver/print-supported-extensions.c should be extended to check at least the column names and 1 row of the extension details. This is because we're now combining a few functions to produce this output, so an integration test is needed.

It is a bit ugly to have to combine these 2 sources of information but that's a larger issue that shouldn't block this.

clang/tools/driver/cc1_main.cpp Outdated Show resolved Hide resolved
clang/tools/driver/cc1_main.cpp Outdated Show resolved Hide resolved
llvm/lib/Support/RISCVISAInfo.cpp Outdated Show resolved Hide resolved
llvm/lib/TargetParser/AArch64TargetParser.cpp Outdated Show resolved Hide resolved
llvm/lib/TargetParser/AArch64TargetParser.cpp Outdated Show resolved Hide resolved
llvm/lib/TargetParser/ARMTargetParser.cpp Outdated Show resolved Hide resolved
llvm/lib/TargetParser/ARMTargetParser.cpp Show resolved Hide resolved
llvm/unittests/TargetParser/TargetParserTest.cpp Outdated Show resolved Hide resolved
llvm/lib/Support/RISCVISAInfo.cpp Outdated Show resolved Hide resolved
@cbalint13
Copy link
Contributor Author

@DavidSpickett

Thank you for the review time, will address all the concerns and be back with a re-review request.

@llvmbot llvmbot added the clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' label Sep 20, 2023
@cbalint13
Copy link
Contributor Author

@DavidSpickett ,

The new output: console-output.txt.gz

$ zcat console-output.txt.gz | grep 'print'
$ ./bin/clang --target=arm-unknown-linux-gnu --print-supported-extensions
$ ./bin/clang --target=aarch64-unknown-linux-gnu --print-supported-extensions
$ ./bin/clang --target=riscv64-unknown-linux-gnu --print-supported-extensions

Addressed all requests, let me know if still spot something.
Re-formatted the print without any '\t', but using spaces with precise justifiers.

llvm/unittests/TargetParser/TargetParserTest.cpp Outdated Show resolved Hide resolved
llvm/lib/TargetParser/ARMTargetParser.cpp Outdated Show resolved Hide resolved
llvm/lib/Support/RISCVISAInfo.cpp Show resolved Hide resolved
llvm/lib/Support/RISCVISAInfo.cpp Show resolved Hide resolved
llvm/lib/TargetParser/ARMTargetParser.cpp Outdated Show resolved Hide resolved
llvm/include/llvm/Support/RISCVISAInfo.h Outdated Show resolved Hide resolved
clang/tools/driver/cc1_main.cpp Outdated Show resolved Hide resolved
@cbalint13
Copy link
Contributor Author

@DavidSpickett ,

Addressed all requests.

$ zcat ~/console-output.txt.gz | grep 'print'
$ ./bin/clang --target=arm-unknown-linux-gnu --print-supported-extensions
$ ./bin/clang --target=aarch64-unknown-linux-gnu --print-supported-extensions
$ ./bin/clang --target=riscv64-unknown-linux-gnu --print-supported-extensions

@DavidSpickett
Copy link
Collaborator

Looks good to me, thanks for working on this.

I'm not sure if you'll have permissions to click the merge button but if you do, go ahead. Please remember to edit the final commit message to remove the links to the output files.

@cbalint13
Copy link
Contributor Author

@DavidSpickett ,

Looks good to me, thanks for working on this.

Thank you very much for the time !

I'm not sure if you'll have permissions to click the merge button but if you do, go ahead.

  • No, don't, I am outside contributor.

Please remember to edit the final commit message to remove the links to the output files.

  • Done.

@DavidSpickett DavidSpickett merged commit 73779bb into llvm:main Sep 22, 2023
@DavidSpickett
Copy link
Collaborator

If you are doing more contributions you can get commit access by doing https://llvm.org/docs/DeveloperPolicy.html#obtaining-commit-access. Though with the switch to Github it's super easy to ask the reviewer to click the merge button, so it's not required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:RISC-V clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category llvm:support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants