khchen added a comment. Herald added subscribers: pcwang-thead, eopXD. In D115921#3206434 <https://reviews.llvm.org/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 possible ideas: > > - Tests with the correct extension versions (maybe add a test file that > exercises the version for all extensions). > - Tests that show an error message with unsupported versions. > - A test that shows that something like mattr=+m,+m2p1 is allowed (or not). > > Nit: fix the lint / no new line warnings. I agree with @luismarques, we need increased test coverage. BTW, I think this patch need to work with D113237 <https://reviews.llvm.org/D113237> to show the ability on supporting multiple extension version. > Anybody else has more comments about support multi-version extension? Or it > has been decided already by RISC-V foundation? IMPO, supporting multi version for ratified extensions in compiler does make sense to me. GCC already did it, but I'm not sure is there any related discussion before when gcc decided to support multi version extension (or misa-spec). ================ Comment at: clang/test/Driver/riscv-arch-version.c:7 +// RUN: FileCheck -check-prefix=RV32-M2P0 %s +// RV32-M2P0: "-target-feature" "+m" + ---------------- I'm thinking do we also need to have `-target-feature +m2p0` here since we are going to support multiple extension version? ================ Comment at: llvm/lib/Target/RISCV/RISCV.td:14 //===----------------------------------------------------------------------===// def FeatureStdExtM ---------------- nit: It will be good to have a comment to talk about what's default version come from? I guess the default is -misa-spec=2.2, right? ================ Comment at: llvm/test/CodeGen/RISCV/attributes-version.ll:3 + +; RUN: llc -mtriple=riscv32 -mattr=+m %s -o - | FileCheck --check-prefix=RV32M %s +; RUN: llc -mtriple=riscv32 -mattr=+a %s -o - | FileCheck --check-prefix=RV32A %s ---------------- For example, we need to have test for `llc -mattr=+m,+m2p0`, and invalid test for `mattr=+m,+m1p9` 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
