kiranchandramohan accepted this revision. kiranchandramohan added a comment.
LG. Please wait for and OK from @jrtc27. ================ Comment at: clang/lib/Driver/ToolChains/Flang.cpp:112-114 + case llvm::Triple::riscv64: + getTargetFeatures(D, Triple, Args, CmdArgs, /*ForAs*/ false); + break; ---------------- jrtc27 wrote: > 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?.... Putting the information in documentation also risks the fact that developers might not see it. Ideally, some portion of the compiler should provide an error if the target or a particular abi is not supported. I think we can discuss this specific issue in a separate patch. 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
