[PATCH] D141307: Add -f[no-]loop-versioning option

[Andrzej Warzynski via Phabricator via cfe-commits]

awarzynski added a comment.

Mats, thanks for working on this! Just a few minor suggestions from me.



================
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:55
 
+static bool shouldLoopVersion(const ArgList &Args) {
+  if (Arg *A = Args.getLastArg(options::OPT_Ofast, options::OPT_O,
----------------
Could you add a short Docstring, pls? In particular, is the logic implemented by this method consistent with GFortran? Are there any external docs that could be referred to here?


================
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:56
+static bool shouldLoopVersion(const ArgList &Args) {
+  if (Arg *A = Args.getLastArg(options::OPT_Ofast, options::OPT_O,
+                               options::OPT_O4, options::OPT_floop_versioning,
----------------
Please rewrite this to use [[ https://llvm.org/docs/CodingStandards.html#use-early-exits-and-continue-to-simplify-code | early exit ]], e.g.:
```
  Arg *LoopVersionOption = Args.getLastArg(options::OPT_Ofast, options::OPT_O,
                               options::OPT_O4, options::OPT_floop_versioning,
                               options::OPT_fno_loop_versioning)
  if !(LoopVersionOption)
    return false;

  // The remaining logic HERE
```


================
Comment at: clang/lib/Driver/ToolChains/Flang.cpp:109-110
   addDebugInfoKind(CmdArgs, DebugInfoKind);
+  if (shouldLoopVersion(Args))
+    CmdArgs.push_back("-fversion-loops-for-stride");
 }
----------------
Would you classify this as a code-gen option? Alongside `-fstack-arrays` and `-flang-experimental-hlfir`? It sound like we could introduce another hook here, `adddCodeGenOptions`?


================
Comment at: flang/test/Driver/version-loops.f90:2
+! Test that flang-new forwards the -f{no-,}version-loops-for-stride 
+! options corredly to flang-new -fc1 for different variants of optimisation
+! and explicit flags.
----------------



================
Comment at: flang/test/Driver/version-loops.f90:5
+
+! RUN: %flang -fsyntax-only -### %s -o %t 2>&1 \
+! RUN:   -O3 \
----------------
[nit] If you are using `-###`, then you can just skip `-fsyntax-only` (it doesn't really matter what "action" is requested from the frontend driver).


================
Comment at: flang/test/Driver/version-loops.f90:33-34
+  
+! CHECK: "-fversion-loops-for-stride"
+! CHECK: "-O3"
+
----------------
Similar suggestion for other `CHECK` lines - this will make the test a bit more explicit about the expected output.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141307/new/

https://reviews.llvm.org/D141307