awarzynski added a comment. Thanks for all the updates @mnadeem! Mostly looks good. A few more nits, but nothing substantial :)
In D137995#3958824 <https://reviews.llvm.org/D137995#3958824>, @mnadeem wrote: > In D137995#3931005 <https://reviews.llvm.org/D137995#3931005>, > @kiranchandramohan wrote: > >> We might need `-fc1` tests as well. > > What kind of tests do you think would be appropriate here? Can you point me > to any examples, maybe from clang? I didn't see any tests in Clang specifically for the frontend driver flags taht are added here (`i.e. `-target-cpu` and `-target-feature`). However, you could add a test for situations like this: `-target-cpu=my-imaginary-cpu` and `-target-fature=my-amazing-non-existent-feature`. ================ Comment at: clang/lib/Driver/ToolChains/Flang.cpp:98 + default: + // Untested for other targets but should work generally. + break; ---------------- [nit] I don't think that this comment contributes much. To me it is self-explanatory that only the triples that are actually present here are tested. Having said that, I don't mind keeping it here. ================ Comment at: clang/lib/Driver/ToolChains/Flang.cpp:100 + break; + case llvm::Triple::aarch64: + case llvm::Triple::x86_64: ---------------- nit ================ Comment at: flang/include/flang/Frontend/TargetOptions.h:25-30 /// Options for controlling the target. Currently this is just a placeholder. /// In the future, we will use this to specify various target options that /// will affect the generated code e.g.: /// * CPU to tune the code for -/// * available CPU/hardware extensions -/// * target specific features to enable/disable /// * options for accelerators (e.g. GPUs) /// * (...) ---------------- I think that we can trim this now. WDYT? ================ Comment at: flang/test/Driver/target-cpu-features.f90:25-26 + +! CHECK-A57: "-triple" "aarch64-unknown-linux-gnu" +! CHECK-A57: "-target-cpu" "cortex-a57" "-target-feature" "+v8a" "-target-feature" "+crc" "-target-feature" "+crypto" "-target-feature" "+fp-armv8" "-target-feature" "+neon" "-target-feature" "+sha2" "-target-feature" "+aes" + ---------------- 1. By adding `-fc1` you make it clear that it's the frontend driver invocation that's being tested. 2. `CHECK-SAME` makes sure the triple is matched with the right set of features. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137995/new/ https://reviews.llvm.org/D137995 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits