[PATCH] D82807: [clang-tidy] Allows the prevailing include header guard in Flang ...

[Eric Schweitz via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGe1581540876f: [clang-tidy] Allows the prevailing include 
header guard in Flang to be… (authored by schweitz).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82807

Files:
  clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp


Index: clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
===
--- clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
+++ clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
@@ -54,6 +54,10 @@
   if (StringRef(Guard).startswith("clang"))
 Guard = "LLVM_" + Guard;
 
+  // The prevalent style in flang is FORTRAN_FOO_BAR_H
+  if (StringRef(Guard).startswith("flang"))
+Guard = "FORTRAN" + Guard.substr(sizeof("flang") - 1);
+
   return StringRef(Guard).upper();
 }
 


Index: clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
===
--- clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
+++ clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
@@ -54,6 +54,10 @@
   if (StringRef(Guard).startswith("clang"))
 Guard = "LLVM_" + Guard;
 
+  // The prevalent style in flang is FORTRAN_FOO_BAR_H
+  if (StringRef(Guard).startswith("flang"))
+Guard = "FORTRAN" + Guard.substr(sizeof("flang") - 1);
+
   return StringRef(Guard).upper();
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82807: [clang-tidy] Allows the prevailing include header guard in Flang ...

[Eric Schweitz via Phabricator via cfe-commits]

schweitz created this revision.
schweitz added reviewers: bkramer, hokein.
schweitz added a project: clang-tools-extra.
Herald added subscribers: cfe-commits, xazax.hun.
Herald added a reviewer: DavidTruby.
Herald added a reviewer: sscalpone.
Herald added a project: clang.

to be recognized rather than flagged as a violation in phabricator.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D82807

Files:
  clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp


Index: clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
===
--- clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
+++ clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
@@ -54,6 +54,10 @@
   if (StringRef(Guard).startswith("clang"))
 Guard = "LLVM_" + Guard;
 
+  // The prevalent style in flang is FORTRAN_FOO_BAR_H
+  if (StringRef(Guard).startswith("flang"))
+Guard = "FORTRAN" + Guard.substr(sizeof("flang")-1);
+
   return StringRef(Guard).upper();
 }
 


Index: clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
===
--- clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
+++ clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
@@ -54,6 +54,10 @@
   if (StringRef(Guard).startswith("clang"))
 Guard = "LLVM_" + Guard;
 
+  // The prevalent style in flang is FORTRAN_FOO_BAR_H
+  if (StringRef(Guard).startswith("flang"))
+Guard = "FORTRAN" + Guard.substr(sizeof("flang")-1);
+
   return StringRef(Guard).upper();
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D82807: [clang-tidy] Allows the prevailing include header guard in Flang ...

[Eric Schweitz via Phabricator via cfe-commits]

schweitz updated this revision to Diff 274278.

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

https://reviews.llvm.org/D82807

Files:
  clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp


Index: clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
===
--- clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
+++ clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
@@ -54,6 +54,10 @@
   if (StringRef(Guard).startswith("clang"))
 Guard = "LLVM_" + Guard;
 
+  // The prevalent style in flang is FORTRAN_FOO_BAR_H
+  if (StringRef(Guard).startswith("flang"))
+Guard = "FORTRAN" + Guard.substr(sizeof("flang") - 1);
+
   return StringRef(Guard).upper();
 }
 


Index: clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
===
--- clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
+++ clang-tools-extra/clang-tidy/llvm/HeaderGuardCheck.cpp
@@ -54,6 +54,10 @@
   if (StringRef(Guard).startswith("clang"))
 Guard = "LLVM_" + Guard;
 
+  // The prevalent style in flang is FORTRAN_FOO_BAR_H
+  if (StringRef(Guard).startswith("flang"))
+Guard = "FORTRAN" + Guard.substr(sizeof("flang") - 1);
+
   return StringRef(Guard).upper();
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120568: [flang][driver] Add support for -S and implement -c/-emit-obj

[Eric Schweitz via Phabricator via cfe-commits]

schweitz added a comment.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120568

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D118985: [flang][driver] Add support for `-emit-mlir`

[Eric Schweitz via Phabricator via cfe-commits]

schweitz accepted this revision.
schweitz added a comment.
This revision is now accepted and ready to land.

LGTM




Comment at: flang/include/flang/Frontend/FrontendActions.h:150
+//===--===//
+class CodeGenAction : public FrontendAction {
+

This appears in a header file. Should there be doxygen comments?



Comment at: flang/include/flang/Frontend/FrontendActions.h:160
+  std::unique_ptr mlirModule_;
+  std::unique_ptr mlirCtx_;
+  /// }

The LLVM coding conventions do not require trailing undescores.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118985

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D124669: [flang][driver] Add support for -save-temps

[Eric Schweitz via Phabricator via cfe-commits]

schweitz added inline comments.



Comment at: clang/include/clang/Driver/Options.td:3989
 Group;
-def save_temps_EQ : Joined<["-", "--"], "save-temps=">, Flags<[CC1Option, 
NoXarchOption]>,
+def save_temps_EQ : Joined<["-", "--"], "save-temps=">, 
Flags<[CC1Option,FlangOption,NoXarchOption]>,
   HelpText<"Save intermediate compilation results.">;

nit: I noticed that this is getting rid of spaces after commas on a few lines. 
Is there any sense of a prior style being used in that regard?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D124669

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143704: [Flang] Part one of Feature List action

[Eric Schweitz via Phabricator via cfe-commits]

schweitz added a comment.

In D143704#4130124 , @jdoerfert wrote:

> In D143704#4130062 , @clementval 
> wrote:
>
>> Where is this coming from? Did I miss where this was discussed.
>
> It's discussed here, for now. :)
>
>> You will not be able to find all not implemented features by going on 
>> through the AST. Most of the not implemented message are triggered by 
>> lowering.
>
> Right. Let's take a step back.
> First, this allows us to identify (and count) features used by an app 
> programmatically. This is in itself useful and we should not intertwine it 
> with the (non-)implemented parts (see my comment from before).
> Next, we want to identify if a "feature" is implemented or not. This is 
> arguably not a straight forward task and also not 100% tied to AST nodes, 
> however, it would be a useful capability for the time being.
> One idea was to extend the `TODO` function to include some tie to a AST 
> entity. That will work sometimes, and sometimes it won't. In either case it 
> is unclear how we can continue lowering (or whatever stage the TODO is in).
> Thus, we might want to manually associate the existing `TODO`s with AST 
> features in a map we maintain. So we run this counting scheme, then lookup 
> each entity in the map, and provide feedback if it is supported, not 
> supported, or partially supported (or unknown).
> All that said, let's separate the concerns into 1) listing of features, and 
> 2) assigning implementation status. This patch does only the former and 
> should be reviewed as such.

For some background context:

TODO is a macro. It had 2 purposes. 1. Developers: easy to grep for and find. 
2. Developers and users: imposing a practice where flang would halt 
(semi-)meaningfully rather than generate garbage output for the user to wade 
through.

It has no association with the parse tree. (Flang doesn't have an AST to my 
knowledge.) The flang parser has its own diagnostics reporting system.

TODO is flexible enough to allow different diagnostic systems to be used, 
different sorts of fatal exits to report different information, etc.

However, it should be considered harmful to make the TODO macro always 
associate with some syntactic source artifact. It would also be unwise to 
redefine TODO from a halt semantics to a "just keep going and see what happens" 
semantics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143704

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D143704: [Flang] Part one of Feature List action

[Eric Schweitz via Phabricator via cfe-commits]

schweitz added a comment.

I should've mentioned in my comment that the TODO macros are meant to be 
deleted. Some day, there will be no more TODOs and there should be a huge 
celebration.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143704

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120246: [flang][driver] Add support for `--target`/`--triple`

[Eric Schweitz via Phabricator via cfe-commits]

schweitz added a comment.

> In D120246#3339289 , @tschuett 
> wrote:
>
>> https://reviews.llvm.org/D117809
>>
>> There was a discussion that `-emit-llvm` is considered a mistake.
>
> Which comment are you referring to specifically? Also, `-emit-llvm` is 
> something that Flang inherits from Clang. I'm merely making it available in 
> `flang-new` rather than re-defining it.

Following clang's precedent with regard to spelling the option `-emit-llvm` 
seems to both follow the principle of least surprise and most obvious for 
sharing common code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120246

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120246: [flang][driver] Add support for `--target`/`--triple`

[Eric Schweitz via Phabricator via cfe-commits]

schweitz added inline comments.



Comment at: clang/include/clang/Driver/Options.td:4815
 
-}
+} // let Flags = [CC1Option, CC1AsOption, NoDriverOption]
+

Is this comment something left over from edits?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120246

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120568: [flang][driver] Add support for -S and implement -c/-emit-obj

[Eric Schweitz via Phabricator via cfe-commits]

schweitz added inline comments.



Comment at: flang/lib/Frontend/FrontendActions.cpp:43
 using namespace Fortran::frontend;
+using namespace llvm;
 

You'll want to keep in mind that some class names are overloaded between llvm 
and Fortran::xyz namespaces and even between Fortran::uvw and Fortran::xyz 
namespaces.



Comment at: flang/lib/Frontend/FrontendActions.cpp:506
+  std::string error;
+  std::string theTriple = llvmModule->getTargetTriple();
+  const llvm::Target *theTarget =





Comment at: flang/lib/Frontend/FrontendActions.cpp:516
+  llvmModule->setDataLayout(TM->createDataLayout());
+  assert(TM && "Failed to create TargetMachine");
+

This assert comes after `TM` is dereferenced in the line above.



Comment at: flang/lib/Frontend/FrontendActions.cpp:556
+  CodeGenPasses.add(createTargetTransformInfoWrapperPass(TargetIRAnalysis()));
+  Triple triple(llvmModule->getTargetTriple());
+  std::unique_ptr TLII =

getTargetTriple() was already saved into `theTriple` variable above.



Comment at: flang/lib/Frontend/FrontendActions.cpp:559
+  std::make_unique(triple);
+  CodeGenPasses.add(new TargetLibraryInfoWrapperPass(*TLII));
+

Do you want to assert on this RAII of `TLII`?



Comment at: flang/lib/Optimizer/Support/FIRContext.cpp:19
 
-static constexpr const char *tripleName = "fir.triple";
+static constexpr const char *tripleName = "llvm.target_triple";
 

Why is this being changed? This is a FIR specific attribute.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120568

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D120568: [flang][driver] Add support for -S and implement -c/-emit-obj

[Eric Schweitz via Phabricator via cfe-commits]

schweitz added inline comments.
Herald added a project: All.



Comment at: flang/lib/Frontend/FrontendActions.cpp:561
+  std::make_unique(triple);
+  CodeGenPasses.add(new llvm::TargetLibraryInfoWrapperPass(*TLII));
+

Got jumbled around somehow. TLII is allocated at line 559-560. Since you are 
checking allocations with assert in other places, why not here? TLII is 
immediately dereferenced on line 561, without an assertion check.



Comment at: flang/lib/Optimizer/Support/FIRContext.cpp:19
 
-static constexpr const char *tripleName = "fir.triple";
+static constexpr const char *tripleName = "llvm.target_triple";
 

awarzynski wrote:
> schweitz wrote:
> > Why is this being changed? This is a FIR specific attribute.
> The driver assumes that when compiling a Fortran file, a valid triple will be 
> present in the generated LLVM IR file. Currently, `fir.tiple` is just dropped 
> and the generated LLVM IR module contains no triple. For `-emit-obj`/`-S` to 
> work, we do need to make sure that a valid triple is actually there.
> 
> We could add logic to translate `fir.triple` to `llvm.target_triple` (in the 
> FIR --> LLVM MLIR translation layer), but why bother if it's not used 
> anywhere? Instead, Flang could use `llvm.target_triple` from the very 
> beginning. This way, no extra functionality is needed (`llvm.target_triple` 
> is preserved when translating from FIR to LLVM MLIR and then included in the 
> generated LLVM IR module).
> 
> AFAIK, `fir-triple` is never used anywhere. In fact, it's already been 
> removed from [[ 
> https://github.com/flang-compiler/f18-llvm-project/blob/fir-dev/flang/lib/Optimizer/Support/FIRContext.cpp#L19
>  | fir-dev ]].
> 
Thanks for the update. I'm glad to see MLIR decided to use my module attribute 
trick here. :)

Given this, there is no reason to "block copy" the name of the attribute over 
to FIR. Just use the `mlir::LLVM::LLVMDialect::getTargetTripleAttrName()` 
method. The FIR attribute can then be retired and removed along with the setter 
and getter.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D120568

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits