[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

khchen added a comment. Herald added subscribers: pcwang-thead, eopXD. In D115921#3206434 , @luismarques wrote: > I think this would benefit from increased test coverage, namely to show that > the mattr command-line options are properly handled. Some

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

zixuan-wu added a comment. In D115921#3224329 , @jrtc27 wrote: > In D115921#3224324 , @zixuan-wu > wrote: > >> In D115921#3224284 , @jrtc27 wrote: >> >>> but also with

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

jrtc27 added inline comments. Comment at: llvm/lib/Support/RISCVISAInfo.cpp:61 {"zvlsseg", RISCVExtensionVersion{0, 10}}, +//{"zvlsseg", RISCVExtensionVersion{0, 7}}, This one too?.. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115921/new/

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

jrtc27 added a comment. In D115921#3224324 , @zixuan-wu wrote: > In D115921#3224284 , @jrtc27 wrote: > >> but also with RISC-V extensions not being changed once ratified any more >> (changes mean new extensions

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

zixuan-wu added inline comments. Comment at: llvm/lib/Support/RISCVISAInfo.cpp:48 {"v", RISCVExtensionVersion{0, 10}}, +//{"v", RISCVExtensionVersion{0, 7}}, {"zba", RISCVExtensionVersion{1, 0}}, jrtc27 wrote: > Don't do this This nit will be

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

zixuan-wu added a comment. In D115921#3224284 , @jrtc27 wrote: > but also with RISC-V extensions not being changed once ratified any more > (changes mean new extensions entirely, not new versions) I don't think so. Or why is there version in RISC-V

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

jrtc27 added a comment. I'm unconvinced about landing something like this until there's an actual use case in the tree. How do we know this will actually work the way we want it to if there's nothing proving it? It's still unclear to me how exactly this is going to be represented in the target

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

zixuan-wu added a comment. ping... CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115921/new/ https://reviews.llvm.org/D115921 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

zixuan-wu updated this revision to Diff 396148. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115921/new/ https://reviews.llvm.org/D115921 Files: clang/lib/Basic/Targets/RISCV.cpp clang/lib/Driver/ToolChains/Arch/RISCV.cpp clang/test/Driver/riscv-arch-version.c

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

zixuan-wu added inline comments. Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:33 + bool operator!=(const RISCVExtensionVersion ) const { +return !operator==(Version); + } craig.topper wrote: > Use `!(*this == Version)` Good taste.

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

zixuan-wu updated this revision to Diff 396147. zixuan-wu edited the summary of this revision. zixuan-wu added a comment. Address all comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115921/new/ https://reviews.llvm.org/D115921 Files: clang/lib/Basic/Targets/RISCV.cpp

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

craig.topper added inline comments. Comment at: llvm/include/llvm/Support/RISCVISAInfo.h:33 + bool operator!=(const RISCVExtensionVersion ) const { +return !operator==(Version); + } Use `!(*this == Version)` Comment at:

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

zixuan-wu added a comment. In D115921#3206156 , @jrtc27 wrote: > Do not bring back V draft 0.7. It is gone, it will never be supported again > by LLVM under that name. The standard extension namespace is reserved for > ratified extensions and

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

luismarques added a comment. I think this would benefit from increased test coverage, namely to show that the mattr command-line options are properly handled. Some possible ideas: - Tests with the correct extension versions (maybe add a test file that exercises the version for all extensions).

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

jrtc27 added a comment. Do not bring back V draft 0.7. It is gone, it will never be supported again by LLVM under that name. The standard extension namespace is reserved for ratified extensions and development snapshots only, not old drafts vendors decided to ship. For those, non-standard

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

zixuan-wu updated this revision to Diff 395803. zixuan-wu edited the summary of this revision. zixuan-wu added a comment. Address comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D115921/new/ https://reviews.llvm.org/D115921 Files: clang/lib/Basic/Targets/RISCV.cpp

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

zixuan-wu added a comment. In D115921#3201346 , @luismarques wrote: >> enable 'm' extension with passing mattr=+m After this patch, it would be >> -mattr=+m2p0. > > It's not obvious to me that support for extension versions should mean or has > to

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

luismarques added a comment. > enable 'm' extension with passing mattr=+m After this patch, it would be > -mattr=+m2p0. It's not obvious to me that support for extension versions should mean or has to mean that we always explicitly specify the version. Why can't we keep supporting the option

[PATCH] D115921: [RISCV] Refactor the RISCV ISA extension info and target features to support multiple extension version

zixuan-wu created this revision. zixuan-wu added reviewers: asb, craig.topper, kito-cheng, luismarques, apazos, jrtc27, Jim, akuegel. Herald added subscribers: VincentWu, luke957, achieveartificialintelligence, armkevincheng, eric-k256, vkmr, frasercrmck, jdoerfert, evandro, sameer.abuasal,