[PATCH] D137995: [Flang][Driver] Handle target CPU and features

[Andrzej Warzynski via Phabricator via cfe-commits]

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