jrtc27 added inline comments.
================ Comment at: clang/lib/Driver/ToolChains/Flang.cpp:112-114 + case llvm::Triple::riscv64: + getTargetFeatures(D, Triple, Args, CmdArgs, /*ForAs*/ false); + break; ---------------- kiranchandramohan wrote: > jrtc27 wrote: > > mnadeem wrote: > > > identical code, could just do a fallthrough > > Why is this even conditional?? > There could be other ways to do this. But it provided a way to understand the > list of supported architectures. Well RISC-V needs -target-abi so its presence in the list does nothing to say it's properly supported. Nor do I think that's a good argument aside from that. You know every architecture needs to have its target features communicated, so making it conditional is not the right technical approach. Documenting the supported architectures should be done another way; perhaps in the actual documentation that developers/users are going to read, rather than buried in code?.... ================ Comment at: flang/test/Driver/code-gen-rv64.f90:7 +! RUN: %flang_fc1 -triple riscv64-unknown-linux-gnu \ +! RUN: -target-feature +d -target-feature +c -emit-obj %s -o %t.o +! RUN: llvm-readelf -h %t.o | FileCheck %s ---------------- Why do we need to go to an object file??? That's terrible practice in Clang tests, and the same should be true of Flang. Test the IR, that is sufficient, and decouples you from the backend. ================ Comment at: flang/test/Driver/target-cpu-features-invalid.f90:13 +! CHECK-INVALID-CPU: 'supercpu' is not a recognized processor for this target (ignoring processor) +! CHECK-INVALID-FEATURE: '+superspeed' is not a recognized feature for this target (ignoring feature) ---------------- Don't these come from the backend? Testing them here doesn't seem right... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145883/new/ https://reviews.llvm.org/D145883 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
