[PATCH] D147218: [OpenMP][Flang][MLIR] Lowering of OpenMP requires directive from parse tree to MLIR

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

In D147218#4645055 , @skatrak wrote:

> Thank you for the review. After I address your last comment my plan is to 
> land this and the other two accepted REQUIRES patches that depended on it.
>
> Is there a preferred approach as to how to go about it? I could rebase them 
> all and wait until the pre-merge builds finish without errors and then land 
> them in quick succession or I could go one by one to make sure post-merge 
> builds don't find any issues before landing the next, which will be over a 
> couple of days most likely.

Both are fine. I would recommend the former.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147218

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

[PATCH] D147218: [OpenMP][Flang][MLIR] Lowering of OpenMP requires directive from parse tree to MLIR

[Kiran Chandramohan via Phabricator via cfe-commits]

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

LG.




Comment at: flang/lib/Lower/Bridge.cpp:2366-2367
 mlir::OpBuilder::InsertPoint insertPt = builder->saveInsertionPoint();
+analyzeOpenMPDeclarativeConstruct(*this, getEval(), ompDecl,
+  ompDeviceCodeFound);
 genOpenMPDeclarativeConstruct(*this, getEval(), ompDecl);

Can this be rewritten this way.

And rename `analyzeOpenMPDeclarativeConstruct` to `isTargetDeclare` or 
something like that?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147218

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

[PATCH] D147218: [OpenMP][Flang][MLIR] Lowering of OpenMP requires directive from parse tree to MLIR

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

Could you add to the summary that the `atomic` related handling is done 
elsewhere.

Could you expand the tests to cover the various `if` conditions that are used 
in the code?




Comment at: flang/include/flang/Lower/OpenMP.h:16
 
+#include "mlir/Dialect/OpenMP/OpenMPDialect.h"
 #include 

Is this include required here?



Comment at: flang/lib/Lower/Bridge.cpp:315
+ [&](Fortran::lower::pft::BlockDataUnit &b) {
+   if (!globalOmpRequiresSymbol)
+ globalOmpRequiresSymbol = b.symTab.symbol();

Is this handling required for `Block Data`? If so, could you add a test?



Comment at: flang/lib/Lower/Bridge.cpp:4779
+if (ompDeviceCodeFound)
+  Fortran::lower::genOpenMPRequires(getModuleOp().getOperation(),
+globalOmpRequiresSymbol);

If this is specific for device code, might be worth renaming it to something 
specific to device.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147218

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

[PATCH] D147219: [OpenMP][Flang][MLIR] Lowering of requires directive from MLIR to LLVM IR

[Kiran Chandramohan via Phabricator via cfe-commits]

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

LG.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147219

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

[PATCH] D159212: [MLIR] Allow dialects to disable CSE for certain operations

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

I think this requires an RFC in MLIR discourse to get some feedback from the 
experts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D159212

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

[PATCH] D158430: [flang][driver] Mark -fuse-ld as visible in Flang

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

@erjin Could you land this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158430

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

[PATCH] D158507: [Flang][Driver] Add support for fomit-frame-pointer

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

For our immediate purpose, I think changing the visibility in the Driver to 
include `flang` is sufficient. After that, we can spend time implementing this 
properly by refactoring the code to llvm.




Comment at: llvm/include/llvm/Frontend/Driver/FramePointer.h:11
+#include "../../../../../clang/include/clang/Driver/Options.h"
+#include "../../../../../clang/lib/Driver/ToolChains/CommonArgs.h"
+#include "llvm/Option/ArgList.h"

We should not include clang headers in llvm. We also should not refer to clang 
inside llvm.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158507

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

[PATCH] D158430: [flang][driver] Mark -fuse-ld as visible in Flang

[Kiran Chandramohan via Phabricator via cfe-commits]

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

Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158430

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

[PATCH] D158309: [flang][driver] Mark -Wl as visible in Flang

[Kiran Chandramohan via Phabricator via cfe-commits]

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

LG.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158309

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

[PATCH] D158289: [flang][driver] Partial revert of https://reviews.llvm.org/D157837

[Kiran Chandramohan via Phabricator via cfe-commits]

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

LG.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158289

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

[PATCH] D147219: [OpenMP][Flang][MLIR] Lowering of requires directive from MLIR to LLVM IR

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added inline comments.



Comment at: mlir/lib/Target/LLVMIR/ModuleTranslation.cpp:1388-1393
+  // Convert module itself before any functions and operations inside, so that
+  // the OpenMPIRBuilder is configured with the OpenMP dialect attributes
+  // attached to the module by `amendOperation()` calls before then.
   llvm::IRBuilder<> llvmBuilder(llvmContext);
+  if (failed(translator.convertOperation(*module, llvmBuilder)))
+return nullptr;

This change has to go in another patch reviewed by the general MLIR/LLVM 
dialect community. Not sure about mentioning too much about OpenMPIRbuilder or 
the OpenMPDialect here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147219

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

[PATCH] D156320: [Flang][Driver] Add support for Rpass and related options

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added inline comments.



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:156-158
+/// Parse a remark command line argument. It may be missing, disabled/enabled 
by
+/// '-R[no-]group' or specified with a regular expression by '-Rgroup=regexp'.
+/// On top of that, it can be disabled/enabled globally by '-R[no-]everything'.

awarzynski wrote:
> Could you give an example (and write a test ) for `-Rgroup=regexp`. Also, 
> @kiranchandramohan , is this form actually needed? I am thinking that it's a 
> complexity that could be avoided. It  could definitely simplify this patch.
`Rpass=regex` is used, particularly `Rpass=.`. So this is required, but can be 
deferred to a separate patch to simplify this one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156320

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

[PATCH] D157410: [Flang][Driver] Enable Rpass and other R family options.

[Kiran Chandramohan via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG6e13e3c3e5e2: [Flang][Driver] Enable Rpass and other R 
family options. (authored by Victor Kingi , 
committed by kiranchandramohan).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157410

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Flang.cpp
  flang/test/Driver/driver-help-hidden.f90
  flang/test/Driver/driver-help.f90
  flang/test/Driver/frontend-forwarding.f90

Index: flang/test/Driver/frontend-forwarding.f90
===
--- flang/test/Driver/frontend-forwarding.f90
+++ flang/test/Driver/frontend-forwarding.f90
@@ -22,6 +22,13 @@
 ! RUN: -fppc-native-vector-element-order \
 ! RUN: -mllvm -print-before-all \
 ! RUN: -save-temps=obj \
+! RUN: -Rpass \
+! RUN: -Rpass-missed \
+! RUN: -Rpass-analysis \
+! RUN: -Rno-pass \
+! RUN: -Reverything \
+! RUN: -Rno-everything \
+! RUN: -Rpass=inline \
 ! RUN: -P \
 ! RUN:   | FileCheck %s
 
@@ -44,5 +51,12 @@
 ! CHECK: "-flang-experimental-hlfir"
 ! CHECK: "-fno-ppc-native-vector-element-order"
 ! CHECK: "-fppc-native-vector-element-order"
+! CHECK: "-Rpass"
+! CHECK: "-Rpass-missed"
+! CHECK: "-Rpass-analysis"
+! CHECK: "-Rno-pass"
+! CHECK: "-Reverything"
+! CHECK: "-Rno-everything"
+! CHECK: "-Rpass=inline"
 ! CHECK: "-mllvm" "-print-before-all"
 ! CHECK: "-save-temps=obj"
Index: flang/test/Driver/driver-help.f90
===
--- flang/test/Driver/driver-help.f90
+++ flang/test/Driver/driver-help.f90
@@ -91,6 +91,10 @@
 ! HELP-NEXT: -print-effective-triple Print the effective target triple
 ! HELP-NEXT: -print-target-triplePrint the normalized target triple
 ! HELP-NEXT: -P Disable linemarker output in -E mode
+! HELP-NEXT: -Rpass-analysis= Report transformation analysis from optimization passes whose name matches the given POSIX regular expression
+! HELP-NEXT: -Rpass-missed=   Report missed transformations by optimization passes whose name matches the given POSIX regular expression
+! HELP-NEXT: -Rpass=  Report transformations performed by optimization passes whose name matches the given POSIX regular expression
+! HELP-NEXT: -R  Enable the specified remark
 ! HELP-NEXT: -save-temps=Save intermediate compilation results.
 ! HELP-NEXT: -save-tempsSave intermediate compilation results
 ! HELP-NEXT: -std=   Language standard to compile for
@@ -214,6 +218,10 @@
 ! HELP-FC1-NEXT: -pic-level   Value for __PIC__
 ! HELP-FC1-NEXT: -plugin  Use the named plugin action instead of the default action (use "help" to list available options)
 ! HELP-FC1-NEXT: -P Disable linemarker output in -E mode
+! HELP-FC1-NEXT: -Rpass-analysis= Report transformation analysis from optimization passes whose name matches the given POSIX regular expression
+! HELP-FC1-NEXT: -Rpass-missed=   Report missed transformations by optimization passes whose name matches the given POSIX regular expression
+! HELP-FC1-NEXT: -Rpass=  Report transformations performed by optimization passes whose name matches the given POSIX regular expression
+! HELP-FC1-NEXT: -R  Enable the specified remark
 ! HELP-FC1-NEXT: -save-temps=Save intermediate compilation results.
 ! HELP-FC1-NEXT: -save-tempsSave intermediate compilation results
 ! HELP-FC1-NEXT: -std=   Language standard to compile for
Index: flang/test/Driver/driver-help-hidden.f90
===
--- flang/test/Driver/driver-help-hidden.f90
+++ flang/test/Driver/driver-help-hidden.f90
@@ -95,6 +95,10 @@
 ! CHECK-NEXT: -print-effective-triple Print the effective target triple
 ! CHECK-NEXT: -print-target-triplePrint the normalized target triple
 ! CHECK-NEXT: -P Disable linemarker output in -E mode
+! CHECK-NEXT: -Rpass-analysis= Report transformation analysis from optimization passes whose name matches the given POSIX regular expression
+! CHECK-NEXT: -Rpass-missed=   Report missed transformations by optimization passes whose name matches the given POSIX regular expression
+! CHECK-NEXT: -Rpass=  Report transformations performed by optimization passes whose name matches the given POSIX regular expression
+! CHECK-NEXT: -R  Enable the specified remark
 ! CHECK-NEXT: -save-temps=Save intermediate compilation results.
 ! CHECK-NEXT: -save-tempsSave intermediate compilation results
 ! CHECK-NEXT: -std=   Language standard to compile for
Index: clang/lib/Driver/ToolChains/Flang.cpp
===
--- clang/lib/Driver/ToolChains/Flang.cpp
+++ clang/lib/Driver/ToolChai

[PATCH] D157493: Revert "Revert "[Flang][Sema] Move directive sets to a shared location""

[Kiran Chandramohan via Phabricator via cfe-commits]

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

LG. Please wait for the CI to pass.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157493

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

[PATCH] D157410: [Flang][Driver] Enable Rpass and other R family options.

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan accepted this revision.
kiranchandramohan added a comment.

LG.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157410

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

[PATCH] D157090: [Flang][Sema] Move directive sets to a shared location

[Kiran Chandramohan via Phabricator via cfe-commits]

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

Thanks. LG.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157090

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

[PATCH] D157090: [Flang][Sema] Move directive sets to a shared location

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

This looks OK. The only concern is whether we will lose the ability to inline 
the code for set membership. Can these sets be put in a header file with 
`inline constexpr`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D157090

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

[PATCH] D156320: [FLang] Add support for Rpass flag

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

> rpass flag now prints remarks when requested but does not display
> the passName used, i.e [-Rpass=inline]

I think the location information is also not printed. Please check the 
difference in implementation of the `TextDiagnosticPrinter::HandleDiagnostic` 
function in clang 
(https://github.com/llvm/llvm-project/blob/7240008c0afa3e2d12f3f51cfe0235668feb6ef3/clang/lib/Frontend/TextDiagnosticPrinter.cpp#L109)
 and flang 
(https://github.com/llvm/llvm-project/blob/7240008c0afa3e2d12f3f51cfe0235668feb6ef3/flang/lib/Frontend/TextDiagnosticPrinter.cpp#L32).
In particular, the passName is printed in the printDiagnosticOptions function 
https://github.com/llvm/llvm-project/blob/7240008c0afa3e2d12f3f51cfe0235668feb6ef3/clang/lib/Frontend/TextDiagnosticPrinter.cpp#L85




Comment at: clang/lib/Basic/DiagnosticIDs.cpp:796-797
 Diag.ErrorOccurred = true;
-if (Diag.Client->IncludeInDiagnosticCounts()) {
+if (Diag.Client->IncludeInDiagnosticCounts())
   ++Diag.NumErrors;
 

Looks like an unrelated change.



Comment at: clang/lib/CodeGen/CodeGenAction.cpp:772
 
-  Diags.Report(Loc, DiagID)
-  << AddFlagValue(D.getPassName())
-  << MsgStream.str();
+  Diags.Report(Loc, DiagID) << AddFlagValue(D.getPassName()) << 
MsgStream.str();
 

Looks like an unrelated change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156320

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

[PATCH] D156524: [flang][nfc] Simplify option forwarding

[Kiran Chandramohan via Phabricator via cfe-commits]

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

LG.




Comment at: clang/lib/Driver/ToolChains/Flang.cpp:146-147
+
+  Args.AddAllArgs(CmdArgs, {options::OPT_flang_experimental_hlfir,
+options::OPT_flang_experimental_polymorphism});
 }

Is the order changing in the tests below (poly before hlfir) expected?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156524

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

[PATCH] D155452: [Flang] Add support for fsave-optimization-record

[Kiran Chandramohan via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGf04ccadf3544: [Flang] Add support for 
fsave-optimization-record (authored by Victor Kingi 
, committed by kiranchandramohan).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155452

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Flang.cpp
  flang/include/flang/Frontend/CodeGenOptions.h
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/FrontendActions.cpp
  flang/test/Driver/driver-help-hidden.f90
  flang/test/Driver/driver-help.f90
  flang/test/Driver/fsave-optimization-record.f90

Index: flang/test/Driver/fsave-optimization-record.f90
===
--- /dev/null
+++ flang/test/Driver/fsave-optimization-record.f90
@@ -0,0 +1,59 @@
+! Tests for the '-f[no-]save-optimization-record[=]' flag.
+
+! Test opt_record flags get generated for fc1
+! RUN: %flang -### %s 2>&1 \
+! RUN: -foptimization-record-file=%t.opt.yaml \
+! RUN:   | FileCheck --check-prefix=YAML %s
+
+! RUN: %flang -### %s 2>&1 \
+! RUN: -fsave-optimization-record \
+! RUN:   | FileCheck --check-prefix=YAML %s
+
+
+! Test -foptimization-record-file produces YAML file with given content
+! RUN: rm -f %t.opt.yaml
+! RUN: %flang -foptimization-record-file=%t.opt.yaml -c %s
+! RUN: cat %t.opt.yaml | FileCheck %s
+
+
+! Test -fsave-optimization-record produces YAML file with given content
+! RUN: rm -f %t.opt.yaml
+! RUN: %flang -fsave-optimization-record -c -o %t.o %s
+! RUN: cat %t.opt.yaml | FileCheck %s
+
+! RUN: rm -f %t.opt.yaml
+! RUN: %flang -fsave-optimization-record -S -o %t.s %s
+! RUN: cat %t.opt.yaml | FileCheck %s
+
+
+! Produces an empty file
+! RUN: rm -f %t.opt.yaml
+! RUN: %flang -fsave-optimization-record -S -emit-llvm -o %t.ll %s
+! RUN: cat %t.opt.yaml
+
+
+!Test unknown format produces error
+! RUN: not %flang -fsave-optimization-record=hello %s 2>&1 \
+! RUN:   | FileCheck --check-prefix=CHECK-FORMAT-ERROR %s
+
+
+! YAML: "-opt-record-file" "{{.+}}.opt.yaml"
+! YAML: "-opt-record-format" "yaml"
+
+! CHECK: --- !Analysis
+! CHECK: Pass:prologepilog
+! CHECK: Name:StackSize
+! CHECK: Function:_QQmain
+! CHECK: Pass:asm-printer
+! CHECK: Name:InstructionMix
+! CHECK: Name:InstructionCount
+
+! CHECK-FORMAT-ERROR: error: unknown remark serializer format: 'hello'
+
+program forttest
+implicit none
+integer :: n
+
+n = 1 * 1
+
+end program forttest
Index: flang/test/Driver/driver-help.f90
===
--- flang/test/Driver/driver-help.f90
+++ flang/test/Driver/driver-help.f90
@@ -54,8 +54,16 @@
 ! HELP-NEXT: -fopenmp-version=
 ! HELP-NEXT:Set OpenMP version (e.g. 45 for OpenMP 4.5, 50 for OpenMP 5.0). Default value is 50 for Clang and 11 for Flang
 ! HELP-NEXT: -fopenmp   Parse OpenMP pragmas and generate parallel code.
+! HELP-NEXT: -foptimization-record-file=
+! HELP-NEXT:Specify the output name of the file containing the optimization remarks. Implies -fsave-optimization-record. On Darwin platforms, this cannot be used with multiple -arch  options.
+! HELP-NEXT: -foptimization-record-passes=
+! HELP-NEXT:Only include passes which match a specified regular expression in the generated optimization record (by default, include all passes)
 ! HELP-NEXT: -fpass-plugin= Load pass plugin from a dynamic shared object file (only with new pass manager).
 ! HELP-NEXT: -freciprocal-math  Allow division operations to be reassociated
+! HELP-NEXT: -fsave-optimization-record=
+! HELP-NEXT:Generate an optimization record file in a specific format
+! HELP-NEXT: -fsave-optimization-record
+! HELP-NEXT:Generate a YAML optimization record file
 ! HELP-NEXT: -fstack-arrays Attempt to allocate array temporaries on the stack, no matter their size
 ! HELP-NEXT: -fsyntax-only  Run the preprocessor, parser and semantic analysis stages
 ! HELP-NEXT: -funderscoring Appends one trailing underscore to external names
@@ -186,6 +194,12 @@
 ! HELP-FC1-NEXT: -mrelocation-model 
 ! HELP-FC1-NEXT:The relocation model to use
 ! HELP-FC1-NEXT: -nocpp Disable predefined and command line preprocessor macros
+! HELP-FC1-NEXT: -opt-record-file 
+! HELP-FC1-NEXT:File name to use for YAML optimization record output
+! HELP-FC1-NEXT: -opt-record-format 
+! HELP-FC1-NEXT:The format used for serializing remarks (default: YAML)
+! HELP-FC1-NEXT: -opt-record-passes 
+! HELP-FC1-NEXT:Only record remark information for passes whose names match the given regular expression
 ! HELP-F

[PATCH] D155452: [Flang] Add support for fsave-optimization-record

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

Please check the failing test.




Comment at: flang/test/Driver/fsave-optimization-record.f90:14
+! RUN: rm -f %t.opt.yaml
+! RUN: %flang -fsave-optimization-record -S -o %t.o %s
+! RUN: cat %t.opt.yaml | FileCheck %s





Comment at: flang/test/Driver/fsave-optimization-record.f90:19
+! RUN: rm -f %t.opt.yaml
+! RUN: %flang -fsave-optimization-record -S -emit-llvm -o %t.o %s
+! RUN: cat %t.opt.yaml




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155452

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

[PATCH] D155279: [Flang] Include logical default with default-integer-8

[Kiran Chandramohan via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGfc43c4f0181b: [Flang] Include logical default with 
default-integer-8 (authored by kiranchandramohan).
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Changed prior to commit:
  https://reviews.llvm.org/D155279?vs=540336&id=541446#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155279

Files:
  clang/include/clang/Driver/Options.td
  flang/docs/IntrinsicTypes.md
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/test/Driver/driver-help-hidden.f90
  flang/test/Driver/driver-help.f90
  flang/test/Lower/Intrinsics/get_environment_variable.f90
  flang/tools/f18-parse-demo/f18-parse-demo.cpp

Index: flang/tools/f18-parse-demo/f18-parse-demo.cpp
===
--- flang/tools/f18-parse-demo/f18-parse-demo.cpp
+++ flang/tools/f18-parse-demo/f18-parse-demo.cpp
@@ -405,6 +405,7 @@
   defaultKinds.set_defaultRealKind(8);
 } else if (arg == "-i8" || arg == "-fdefault-integer-8") {
   defaultKinds.set_defaultIntegerKind(8);
+  defaultKinds.set_defaultLogicalKind(8);
 } else if (arg == "-help" || arg == "--help" || arg == "-?") {
   llvm::errs()
   << "f18-parse-demo options:\n"
Index: flang/test/Lower/Intrinsics/get_environment_variable.f90
===
--- flang/test/Lower/Intrinsics/get_environment_variable.f90
+++ flang/test/Lower/Intrinsics/get_environment_variable.f90
@@ -82,7 +82,8 @@
 
 ! CHECK-LABEL: func @_QPname_and_trim_name_only(
 ! CHECK-SAME: %[[nameArg:.*]]: !fir.boxchar<1> {fir.bindc_name = "name"},
-! CHECK-SAME: %[[trimNameArg:.*]]: !fir.ref> {fir.bindc_name = "trim_name"}) {
+! CHECK-32-SAME: %[[trimNameArg:.*]]: !fir.ref> {fir.bindc_name = "trim_name"}) {
+! CHECK-64-SAME: %[[trimNameArg:.*]]: !fir.ref> {fir.bindc_name = "trim_name"}) {
 subroutine name_and_trim_name_only(name, trim_name)
 character(len=32) :: name
 logical :: trim_name
@@ -120,7 +121,8 @@
 ! CHECK-SAME: %[[valueArg:.*]]: !fir.boxchar<1> {fir.bindc_name = "value"},
 ! CHECK-SAME: %[[lengthArg:[^:]*]]: !fir.ref {fir.bindc_name = "length"},
 ! CHECK-SAME: %[[statusArg:.*]]: !fir.ref {fir.bindc_name = "status"},
-! CHECK-SAME: %[[trimNameArg:.*]]: !fir.ref> {fir.bindc_name = "trim_name"},
+! CHECK-32-SAME: %[[trimNameArg:.*]]: !fir.ref> {fir.bindc_name = "trim_name"},
+! CHECK-64-SAME: %[[trimNameArg:.*]]: !fir.ref> {fir.bindc_name = "trim_name"},
 ! CHECK-SAME: %[[errmsgArg:.*]]: !fir.boxchar<1> {fir.bindc_name = "errmsg"}) {
 subroutine all_arguments(name, value, length, status, trim_name, errmsg)
 character(len=32) :: name, value, errmsg
@@ -138,15 +140,17 @@
 ! CHECK-NEXT: %[[lengthBoxed:.*]] = fir.embox %[[lengthArg]] : (!fir.ref) -> !fir.box
 ! CHECK-NEXT: %[[errmsgBoxed:.*]] = fir.embox %[[errmsgCast]] : (!fir.ref>) -> !fir.box>
 ! CHECK:  %[[trimName:.*]] = fir.if %{{.*}} -> (i1) {
-! CHECK-NEXT:   %[[trimNameLoaded:.*]] = fir.load %[[trimNameArg]] : !fir.ref>
-! CHECK-NEXT:   %[[trimCast:.*]] = fir.convert %[[trimNameLoaded]] : (!fir.logical<4>) -> i1
+! CHECK-32-NEXT:   %[[trimNameLoaded:.*]] = fir.load %[[trimNameArg]] : !fir.ref>
+! CHECK-64-NEXT:   %[[trimNameLoaded:.*]] = fir.load %[[trimNameArg]] : !fir.ref>
+! CHECK-32-NEXT:   %[[trimCast:.*]] = fir.convert %[[trimNameLoaded]] : (!fir.logical<4>) -> i1
+! CHECK-64-NEXT:   %[[trimCast:.*]] = fir.convert %[[trimNameLoaded]] : (!fir.logical<8>) -> i1
 ! CHECK-NEXT:   fir.result %[[trimCast]] : i1
 ! CHECK-NEXT: } else {
 ! CHECK-NEXT:   %[[trueVal:.*]] = arith.constant true
 ! CHECK-NEXT:   fir.result %[[trueVal]] : i1
 ! CHECK-NEXT: }
 ! CHECK: %[[sourceFileString:.*]] = fir.address_of(@_QQcl.[[fileString:.*]]) : !fir.ref>
-! CHECK-NEXT: %[[sourceLine:.*]] = arith.constant [[# @LINE - 20]] : i32
+! CHECK-NEXT: %[[sourceLine:.*]] = arith.constant [[# @LINE - 22]] : i32
 ! CHECK-NEXT: %[[name:.*]] = fir.convert %[[nameBoxed]] : (!fir.box>) -> !fir.box
 ! CHECK-NEXT: %[[value:.*]] = fir.convert %[[valueBoxed]] : (!fir.box>) -> !fir.box
 ! CHECK-NEXT: %[[length:.*]] = fir.convert %[[lengthBoxed]] : (!fir.box) -> !fir.box
Index: flang/test/Driver/driver-help.f90
===
--- flang/test/Driver/driver-help.f90
+++ flang/test/Driver/driver-help.f90
@@ -27,7 +27,7 @@
 ! HELP-NEXT: -fcolor-diagnosticsEnable colors in diagnostics
 ! HELP-NEXT: -fconvert=  Set endian conversion of data for unformatted files
 ! HELP-NEXT: -fdefault-double-8 Set the default double precision kind to an 8 byte wide type
-! HELP-NEXT: -fdefault-integer-8Set the default integer kind to an 8 byte wide type
+! HELP-NEXT: -fdefault-integer-8Set the default integer and logical kind to an 8 byte wide type
 ! HELP-NEXT: -fdefault-real-8   Set the default real kind to an 8 b

[PATCH] D155452: [Flang] Add support for fsave-optimization-record

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

Couple of quick comments.

You probably need another frontend-forwarding test to check 
`fsave-optimization-record=`.




Comment at: clang/lib/Driver/ToolChains/Flang.cpp:392-395
+static void renderRemarksOptions(const ArgList &Args, ArgStringList &CmdArgs,
+ const llvm::Triple &Triple,
+ const InputInfo &Input,
+ const InputInfo &Output, const JobAction &JA) 
{

JA and Triple do not seem to be used.

If the functionalities are similar, you could move the `renderRemarksOptions` 
from Clang to a file in `llvm/Frontend/` and then use that in Flang.



Comment at: flang/test/Driver/fsave-optimization-record.f90:8-11
+! RUN: %flang -fsave-optimization-record=yaml %s
+! RUN: %flang -fsave-optimization-record -S %s
+! RUN: %flang -fsave-optimization-record -S -emit-llvm %s
+! RUN: %flang -fsave-optimization-record -c %s

Check for the existence of the optimization report in all these cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D155452

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

[PATCH] D147218: [OpenMP][Flang][MLIR] Lowering of OpenMP requires directive from parse tree to MLIR

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added inline comments.



Comment at: flang/lib/Lower/Bridge.cpp:271
+bridge{bridge}, foldingContext{bridge.createFoldingContext()},
+ompRequiresFlags{mlir::omp::ClauseRequires::none} {}
   virtual ~FirConverter() = default;

Is this set anywhere? If not is it required? Can this be made available through 
the bridge?



Comment at: flang/lib/Lower/Bridge.cpp:2234
+  /// Extract information from OpenMP declarative constructs
+  void analyzeOpenMPDeclarative(
+  const Fortran::parser::OpenMPDeclarativeConstruct &ompDecl) {

Can this function be moved to OpenMP.cpp?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147218

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

[PATCH] D150354: [OpenMP][MLIR][Flang][bbc][Driver] Add fopenmp-version and generate corresponding MLIR attribute

[Kiran Chandramohan via Phabricator via cfe-commits]

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

LGTM.

Target-related constructs came in as part of OpenMP 4.0. Would you want to have 
a different default for the device version?


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

https://reviews.llvm.org/D150354

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

[PATCH] D150354: [OpenMP][MLIR][Flang][bbc][Driver] Add fopenmp-version and generate corresponding MLIR attribute

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

In D150354#4342146 , @domada wrote:

> In D150354#4337148 , @awarzynski 
> wrote:
>
>> All in all LGTM, but I'm not sure whether Flang should be defaulting to 
>> OpenMP 5.0. AFAIK, that's not supported yet.
>
> If you wish I can set to OpenMP 4.5. But then we need to have two separate 
> flags in clang/include/clang/Driver/Options.td (one for clang and the second 
> one for flang).

We will not be able to match the OpenMP support in clang (stand support level) 
soon. Although we have made lot of progress, we are effectively around 1.1. To 
stay true to the meaning of this metadata, we will have to have separate flags. 
But, I guess, `fclang-openmp-version` might not be acceptable to Clang folks 
who are used to using `-fopenmp-version`. Can't this be achieved by the same 
flag? On a cursory look, i don't see anything that prevents using the same flag 
but with different defaults in the code that is handling the flag.


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

https://reviews.llvm.org/D150354

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

[PATCH] D148370: [Clang][Flang][OpenMP] Add loadOffloadInfoMetadata and createOffloadEntriesAndInfoMetadata into OMPIRBuilder's finalize and initialize

[Kiran Chandramohan via Phabricator via cfe-commits]

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

LG. Please wait a day before submitting.




Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:455-468
+  auto Buf = MemoryBuffer::getFile(HostFilePath);
+  if (auto Err = Buf.getError())
+report_fatal_error(("error opening host file from host file path inside of 
"
+"OpenMPIRBuilder: " +
+Err.message())
+   .c_str());
+

Nit: Might be good to spell the types and use braces for multi-line code inside 
`if`.



Comment at: mlir/include/mlir/Target/LLVMIR/ModuleTranslation.h:18
 #include "mlir/Dialect/LLVMIR/LLVMInterfaces.h"
+#include "mlir/Dialect/OpenMP/OpenMPDialect.h"
 #include "mlir/IR/Operation.h"

Nit: Is this required here? Can it be in the CPP File?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148370

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

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

In D146557#4301955 , @TIFitis wrote:

> In D146557#4295550 , 
> @kiranchandramohan wrote:
>
>> In D146557#4292223 , @TIFitis 
>> wrote:
>>
>>> Cleaned up how IsBegin argument is passed for createTargetData call.
>>
>> Please submit this directly as an NFC patch.
>
> I have merged it separately.
>
>> We should always work towards simple patches that are easy to review. 
>> Sometimes a big patch is required to give the full context. But it can then 
>> be broken up into small patches for faster review. I have created D149153 
>>  for the `inlineConvertOmpRegions` change.
>
> Thanks for creating your patch. I'll try to make my patches shorter in the 
> future.
>
> Once D149153  is merged I'll rebase this 
> patch.

I have submitted D149153 . 
https://github.com/llvm/llvm-project/commit/1ed522623d95ce7dcf95e711b0f2e3844d2e6be1


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

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

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

In D146557#4292223 , @TIFitis wrote:

> Cleaned up how IsBegin argument is passed for createTargetData call.

Please submit this directly as an NFC patch.

We should always work towards simple patches that are easy to review. Sometimes 
a big patch is required to give the full context. But it can then be broken up 
into small patches for faster review. I have created D149153 
 for the `inlineConvertOmpRegions` change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

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

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

Would it be possible to break this into a few patches like suggested below or 
similar?

1. Changed to using inlineConvertOmpRegions to generate target data associated 
region code inline in same block if possible.

2. Changed to calculating map_operand size from mlir type instead of llvm.

Moved getSizeInBytes function back into OpenACC code as it is no longer shared.

3. Moved most of map_operands related codegen to OMPIRBuilder.

Moved mapper_allocas related codegen to OMPIRBuilder.

4. Changed offload_sizes parameter to a global variable same as 
offload_maptypes to be more consistent with Clang generated code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

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

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added inline comments.



Comment at: mlir/test/Target/LLVMIR/omptarget-llvm.mlir:4-5
-llvm.func @_QPopenmp_target_data() {
-  %0 = llvm.mlir.constant(1 : i64) : i64
-  %1 = llvm.alloca %0 x i32 {bindc_name = "i", in_type = i32, 
operand_segment_sizes = array, uniq_name = 
"_QFopenmp_target_dataEi"} : (i64) -> !llvm.ptr
   omp.target_data   map((tofrom -> %1 : !llvm.ptr)) {

TIFitis wrote:
> kiranchandramohan wrote:
> > Why is the test changed?
> Moved the map_operand to function parameter to be consistent with other 
> tests. I think it can remain unchanged if you prefer.
Consistency can be achieved in a follow-up NFC patch that you can submit 
without review. If the test can only show the differences of this patch it is 
easier to review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

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

[PATCH] D146557: [MLIR][OpenMP] Refactoring how map clause is processed

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

Please update the summary to convey all the changes introduced in the patch.

From the tests, it looks like there is a substantial change in the IR. I was 
hoping this to be an NFC change.




Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1383
   struct MapperAllocas &MapperAllocas, int64_t DeviceID,
-  unsigned NumOperands);
+  unsigned NumOperands, Value *MapsizesArg = nullptr);
 

Please add the new argument to the documentation above.



Comment at: mlir/test/Target/LLVMIR/omptarget-llvm.mlir:4-5
-llvm.func @_QPopenmp_target_data() {
-  %0 = llvm.mlir.constant(1 : i64) : i64
-  %1 = llvm.alloca %0 x i32 {bindc_name = "i", in_type = i32, 
operand_segment_sizes = array, uniq_name = 
"_QFopenmp_target_dataEi"} : (i64) -> !llvm.ptr
   omp.target_data   map((tofrom -> %1 : !llvm.ptr)) {

Why is the test changed?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146557

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

[PATCH] D147324: [OpenMP][MLIR][Flang][bbc][Driver] Add OpenMP RTL Flags to Flang and generate omp.FlagsAttr from them

[Kiran Chandramohan via Phabricator via cfe-commits]

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

LG. This portion of the patch was accepted in https://reviews.llvm.org/D145264.

See inline comment about location of LLVM Dialect tests. You may either remove 
the LLVM dialect tests from this patch and land it. Or you can submit this and 
move the tests in a separate NFC patch and submit without review.




Comment at: flang/test/Lower/OpenMP/rtl-flags.f90:10-27
+!RUN: %flang_fc1 -emit-fir -fopenmp %s -o - | fir-opt --fir-to-llvm-ir | 
FileCheck %s  --check-prefix=DEFAULT-HOST-LLVMDIALECT
+!RUN: %flang_fc1 -emit-fir -fopenmp -fopenmp-is-device %s -o - | fir-opt 
--fir-to-llvm-ir | FileCheck %s --check-prefix=DEFAULT-DEVICE-LLVMDIALECT
+!RUN: %flang_fc1 -emit-fir -fopenmp -fopenmp-target-debug -fopenmp-is-device 
%s -o - | fir-opt --fir-to-llvm-ir | FileCheck %s 
--check-prefix=DBG-DEVICE-LLVMDIALECT
+!RUN: %flang_fc1 -emit-fir -fopenmp -fopenmp-target-debug=111 
-fopenmp-is-device %s -o - | fir-opt --fir-to-llvm-ir | FileCheck %s 
--check-prefix=DBG-EQ-DEVICE-LLVMDIALECT
+!RUN: %flang_fc1 -emit-fir -fopenmp -fopenmp-assume-teams-oversubscription 
-fopenmp-is-device %s -o - | fir-opt --fir-to-llvm-ir | FileCheck %s 
--check-prefix=TEAMS-OSUB-DEVICE-LLVMDIALECT
+!RUN: %flang_fc1 -emit-fir -fopenmp -fopenmp-assume-threads-oversubscription 
-fopenmp-is-device %s -o - | fir-opt --fir-to-llvm-ir | FileCheck %s 
--check-prefix=THREAD-OSUB-DEVICE-LLVMDIALECT
+!RUN: %flang_fc1 -emit-fir -fopenmp -fopenmp-assume-no-thread-state 
-fopenmp-is-device %s -o - | fir-opt --fir-to-llvm-ir | FileCheck %s 
--check-prefix=THREAD-STATE-DEVICE-LLVMDIALECT

The tests for LLVM dialect should be in 
https://github.com/llvm/llvm-project/blob/main/flang/test/Fir/convert-to-llvm-openmp-and-fir.fir.

Same for other LLVM dialect RUN lines below.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147324

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

[PATCH] D145264: [OpenMP][MLIR][Flang][Driver][bbc] Lower and apply Module FlagsAttr

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan requested changes to this revision.
kiranchandramohan added a comment.
This revision now requires changes to proceed.

Please split this patch into three:

1. Code changes and testing for the driver and the FIR+OpenMP dialect generated.
2. Code changes and test for FIR+OpenMP to LLVM+OpenMP dialect.
3. Code changes and testing for the translation from LLVM + OpenMP dialect to 
LLVM IR. Code in OpenMPToLLVMIRTranslation.cpp should be tested with 
`mlir-translate`.

In D145264#4233358 , @agozillon wrote:

> Unfortunately I do not believe an mlir-translate test that tests if the 
> OffloadModuleInterface is accessible when directly utilizing mlir-translate 
> is possible for this patch... I forgot I removed the is device check as it is 
> already done at the initial creation of the attribute. However, I do have a 
> future patch that it will be utilised in and when @jsjodin's initial TargetOp 
> work is in I can likely create a test around that functionality.

You may delay the translation code till then.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145264

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

[PATCH] D142347: [NFC][Clang] Move DebugOptions to llvm/Frontend for reuse in Flang

[Kiran Chandramohan via Phabricator via cfe-commits]

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

LGTM. This refactoring helps share code between Clang and Flang. Please wait a 
couple of days before submitting.

Additional notes in support of approval:
-> The llvm community approved @jdoerfert 's proposal for llvm/lib/Frontend as 
a common directory for sharing code between multiple frontends. 
https://discourse.llvm.org/t/rfc-create-llvm-lib-frontend/53681
-> In general the flang community would like to have the Flang driver 
independent of Clang. I believe the Clang project is fine with this as OKed in 
a reply to a previous RFC 
(https://lists.llvm.org/pipermail/cfe-dev/2020-August/066488.html)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142347

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

[PATCH] D146814: [Flang] Add debug flag to enable current debug information pass

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added inline comments.



Comment at: flang/include/flang/Frontend/CodeGenOptions.h:18
 
+#include "clang/Basic/DebugInfoOptions.h"
 #include "llvm/Support/CodeGen.h"

The following patch might be useful to avoid adding the clang header. Could you 
try it locally and see whether it works? If so you can either commandeer the 
following patch or alternatively include the following patch as part of the 
current one.
https://reviews.llvm.org/D142347


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146814

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

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan accepted this revision.
kiranchandramohan added a comment.
This revision is now accepted and ready to land.
Herald added a subscriber: jplehr.

LGTM. Please wait a day before submission. Move the flang portion to a separate 
commit.




Comment at: flang/lib/Lower/OpenMP.cpp:727
 for (mlir::Value mapOp : mapOperand) {
+  auto checkType = mapOp.getType();
+  if (auto refType = checkType.dyn_cast())

kiranchandramohan wrote:
> These changes should go in a separate patch.
Nit: expand auto.



Comment at: flang/lib/Lower/OpenMP.cpp:727-738
+  auto checkType = mapOp.getType();
+  if (auto refType = checkType.dyn_cast())
+checkType = refType.getElementType();
+  if (auto boxType = checkType.dyn_cast())
+checkType = boxType.getElementType();
+
+  if (!(fir::isa_std_type(checkType) ||

These changes should go in a separate patch.



Comment at: flang/lib/Lower/OpenMP.cpp:737
+ !checkType.cast().hasDynamicExtents(
+TODO(currentLocation, "OMPD_target_data MapOperand type not 
supported");
+

Since TODO will print a "Not yet implemented" message, I think we can remove 
the trailing "not supported suffix".



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1523
+  auto bodyCB = [&](InsertPointTy allocaIP, InsertPointTy codeGenIP) {
+// DataOp has only one region associated with it.
+auto ®ion = cast(op).getRegion();

Nit: I think this should be an assertion.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1502-1510
+  llvm::OpenMPIRBuilder::LocationDescription ompLoc(builder);
+  llvm::OpenMPIRBuilder::InsertPointTy allocaIP =
+  findAllocaInsertPoint(builder, moduleTranslation);
+
+  struct llvm::OpenMPIRBuilder::MapperAllocas mapperAllocas;
+  SmallVector mapTypeFlags;
+  SmallVector mapNames;

TIFitis wrote:
> kiranchandramohan wrote:
> > Can all this go into the OpenMP IRBuilder? And be passed back if necessary 
> > via the callback.
> If we decide to move processMapOperand then these can be moved along with it.
Now that we decided not to move `processMapOperand`, can this be moved to the 
OpenMPIRBuilder?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

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

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1357
+/// Process MapOperands for Target Data directives.
+static LogicalResult processMapOperand(
+llvm::IRBuilderBase &builder, LLVM::ModuleTranslation &moduleTranslation,

TIFitis wrote:
> TIFitis wrote:
> > TIFitis wrote:
> > > kiranchandramohan wrote:
> > > > kiranchandramohan wrote:
> > > > > TIFitis wrote:
> > > > > > TIFitis wrote:
> > > > > > > kiranchandramohan wrote:
> > > > > > > > TIFitis wrote:
> > > > > > > > > kiranchandramohan wrote:
> > > > > > > > > > TIFitis wrote:
> > > > > > > > > > > kiranchandramohan wrote:
> > > > > > > > > > > > Isn't it possible to sink this whole function into the 
> > > > > > > > > > > > OpenMPIRBuilder by passing it a list of `mapOpValue` 
> > > > > > > > > > > > and `mapTypeFlags`?
> > > > > > > > > > > > `lvm::Value *mapOpValue = 
> > > > > > > > > > > > moduleTranslation.lookupValue(mapOp);`
> > > > > > > > > > > > 
> > > > > > > > > > > > Did i miss something? Or is this in anticipation of 
> > > > > > > > > > > > more processing required for other types?
> > > > > > > > > > > I'm not fully sure but we might need more MLIR related 
> > > > > > > > > > > things when supporting types other than LLVMPointerType. 
> > > > > > > > > > > Also there is a call to 
> > > > > > > > > > > mlir::LLVM::createMappingInformation.
> > > > > > > > > > > 
> > > > > > > > > > > I guess it might still be possible to move most of it to 
> > > > > > > > > > > the IRBuilder, would you like me to do that?
> > > > > > > > > > Callbacks are useful when there is frontend-specific 
> > > > > > > > > > handling that is required. If more types require to be 
> > > > > > > > > > handled then it is better to have the callback. We can 
> > > > > > > > > > revisit this after all types are handled. I assume, the 
> > > > > > > > > > current handling is for scalars and arrays of known-size.
> > > > > > > > > I am a novice at FORTRAN so I'm not aware of all  the types 
> > > > > > > > > and scenarios.
> > > > > > > > > 
> > > > > > > > > I've tested the following cases and they work end-to-end:
> > > > > > > > > 
> > > > > > > > > **Fortran:**
> > > > > > > > > ```
> > > > > > > > > subroutine openmp_target_data_region(a)
> > > > > > > > > real :: a(*)
> > > > > > > > > integer :: b(1024)
> > > > > > > > > character :: c
> > > > > > > > > integer, pointer :: p
> > > > > > > > > !$omp target enter data map(to: a, b, c, p)
> > > > > > > > > end subroutine openmp_target_data_region
> > > > > > > > > ```
> > > > > > > > > 
> > > > > > > > > **LLVM IR(** `flang-new -fc1 -emit-llvm -fopenmp test.f90 -o 
> > > > > > > > > test.ll`** ):**
> > > > > > > > > 
> > > > > > > > > ```
> > > > > > > > > ; ModuleID = 'FIRModule'
> > > > > > > > > source_filename = "FIRModule"
> > > > > > > > > target datalayout = 
> > > > > > > > > "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
> > > > > > > > > target triple = "x86_64-unknown-linux-gnu"
> > > > > > > > > 
> > > > > > > > > %struct.ident_t = type { i32, i32, i32, i32, ptr }
> > > > > > > > > 
> > > > > > > > > @0 = private unnamed_addr constant [13 x i8] 
> > > > > > > > > c"loc(unknown)\00", align 1
> > > > > > > > > @1 = private unnamed_addr constant [56 x i8] 
> > > > > > > > > c";/home/akash/Documents/scratch/test2.f90;unknown;3;16;;\00",
> > > > > > > > >  align 1
> > > > > > > > > @2 = private unnamed_addr constant [56 x i8] 
> > > > > > > > > c";/home/akash/Documents/scratch/test2.f90;unknown;4;18;;\00",
> > > > > > > > >  align 1
> > > > > > > > > @3 = private unnamed_addr constant [56 x i8] 
> > > > > > > > > c";/home/akash/Documents/scratch/test2.f90;unknown;5;25;;\00",
> > > > > > > > >  align 1
> > > > > > > > > @4 = private unnamed_addr constant [23 x i8] 
> > > > > > > > > c";unknown;unknown;0;0;;\00", align 1
> > > > > > > > > @5 = private unnamed_addr constant %struct.ident_t { i32 0, 
> > > > > > > > > i32 2, i32 0, i32 22, ptr @4 }, align 8
> > > > > > > > > @.offload_maptypes = private unnamed_addr constant [4 x i64] 
> > > > > > > > > [i64 1, i64 1, i64 1, i64 1]
> > > > > > > > > @.offload_mapnames = private constant [4 x ptr] [ptr @0, ptr 
> > > > > > > > > @1, ptr @2, ptr @3]
> > > > > > > > > 
> > > > > > > > > declare ptr @malloc(i64)
> > > > > > > > > 
> > > > > > > > > declare void @free(ptr)
> > > > > > > > > 
> > > > > > > > > define void @openmp_target_data_region_(ptr %0) {
> > > > > > > > >   %2 = alloca [4 x ptr], align 8
> > > > > > > > >   %3 = alloca [4 x ptr], align 8
> > > > > > > > >   %4 = alloca [4 x i64], align 8
> > > > > > > > >   %5 = alloca [1024 x i32], i64 1, align 4
> > > > > > > > >   %6 = alloca [1 x i8], i64 1, align 1
> > > > > > > > >   %7 = alloca { ptr, i64, i32, i8, i8, i8, i8 }, i64 1, align 
> > > > > > > > > 8
> > > > > > > > >   %8 = alloca ptr, i64 1, align 8
> > > > > > > > >   store ptr null,

[PATCH] D145883: [Flang][RISCV] Emit target features for RISC-V

[Kiran Chandramohan via Phabricator via cfe-commits]

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

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added inline comments.
Herald added a subscriber: sunshaoce.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1357
+/// Process MapOperands for Target Data directives.
+static LogicalResult processMapOperand(
+llvm::IRBuilderBase &builder, LLVM::ModuleTranslation &moduleTranslation,

kiranchandramohan wrote:
> TIFitis wrote:
> > TIFitis wrote:
> > > kiranchandramohan wrote:
> > > > TIFitis wrote:
> > > > > kiranchandramohan wrote:
> > > > > > TIFitis wrote:
> > > > > > > kiranchandramohan wrote:
> > > > > > > > Isn't it possible to sink this whole function into the 
> > > > > > > > OpenMPIRBuilder by passing it a list of `mapOpValue` and 
> > > > > > > > `mapTypeFlags`?
> > > > > > > > `lvm::Value *mapOpValue = moduleTranslation.lookupValue(mapOp);`
> > > > > > > > 
> > > > > > > > Did i miss something? Or is this in anticipation of more 
> > > > > > > > processing required for other types?
> > > > > > > I'm not fully sure but we might need more MLIR related things 
> > > > > > > when supporting types other than LLVMPointerType. Also there is a 
> > > > > > > call to mlir::LLVM::createMappingInformation.
> > > > > > > 
> > > > > > > I guess it might still be possible to move most of it to the 
> > > > > > > IRBuilder, would you like me to do that?
> > > > > > Callbacks are useful when there is frontend-specific handling that 
> > > > > > is required. If more types require to be handled then it is better 
> > > > > > to have the callback. We can revisit this after all types are 
> > > > > > handled. I assume, the current handling is for scalars and arrays 
> > > > > > of known-size.
> > > > > I am a novice at FORTRAN so I'm not aware of all  the types and 
> > > > > scenarios.
> > > > > 
> > > > > I've tested the following cases and they work end-to-end:
> > > > > 
> > > > > **Fortran:**
> > > > > ```
> > > > > subroutine openmp_target_data_region(a)
> > > > > real :: a(*)
> > > > > integer :: b(1024)
> > > > > character :: c
> > > > > integer, pointer :: p
> > > > > !$omp target enter data map(to: a, b, c, p)
> > > > > end subroutine openmp_target_data_region
> > > > > ```
> > > > > 
> > > > > **LLVM IR(** `flang-new -fc1 -emit-llvm -fopenmp test.f90 -o 
> > > > > test.ll`** ):**
> > > > > 
> > > > > ```
> > > > > ; ModuleID = 'FIRModule'
> > > > > source_filename = "FIRModule"
> > > > > target datalayout = 
> > > > > "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
> > > > > target triple = "x86_64-unknown-linux-gnu"
> > > > > 
> > > > > %struct.ident_t = type { i32, i32, i32, i32, ptr }
> > > > > 
> > > > > @0 = private unnamed_addr constant [13 x i8] c"loc(unknown)\00", 
> > > > > align 1
> > > > > @1 = private unnamed_addr constant [56 x i8] 
> > > > > c";/home/akash/Documents/scratch/test2.f90;unknown;3;16;;\00", align 1
> > > > > @2 = private unnamed_addr constant [56 x i8] 
> > > > > c";/home/akash/Documents/scratch/test2.f90;unknown;4;18;;\00", align 1
> > > > > @3 = private unnamed_addr constant [56 x i8] 
> > > > > c";/home/akash/Documents/scratch/test2.f90;unknown;5;25;;\00", align 1
> > > > > @4 = private unnamed_addr constant [23 x i8] 
> > > > > c";unknown;unknown;0;0;;\00", align 1
> > > > > @5 = private unnamed_addr constant %struct.ident_t { i32 0, i32 2, 
> > > > > i32 0, i32 22, ptr @4 }, align 8
> > > > > @.offload_maptypes = private unnamed_addr constant [4 x i64] [i64 1, 
> > > > > i64 1, i64 1, i64 1]
> > > > > @.offload_mapnames = private constant [4 x ptr] [ptr @0, ptr @1, ptr 
> > > > > @2, ptr @3]
> > > > > 
> > > > > declare ptr @malloc(i64)
> > > > > 
> > > > > declare void @free(ptr)
> > > > > 
> > > > > define void @openmp_target_data_region_(ptr %0) {
> > > > >   %2 = alloca [4 x ptr], align 8
> > > > >   %3 = alloca [4 x ptr], align 8
> > > > >   %4 = alloca [4 x i64], align 8
> > > > >   %5 = alloca [1024 x i32], i64 1, align 4
> > > > >   %6 = alloca [1 x i8], i64 1, align 1
> > > > >   %7 = alloca { ptr, i64, i32, i8, i8, i8, i8 }, i64 1, align 8
> > > > >   %8 = alloca ptr, i64 1, align 8
> > > > >   store ptr null, ptr %8, align 8
> > > > >   br label %entry
> > > > > 
> > > > > entry:; preds = %1
> > > > >   %9 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 0
> > > > >   store ptr %0, ptr %9, align 8
> > > > >   %10 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 0
> > > > >   store ptr %0, ptr %10, align 8
> > > > >   %11 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 0
> > > > >   store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to 
> > > > > i64), ptr %11, align 8
> > > > >   %12 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 1
> > > > >   store ptr %5, ptr %12, align 8
> > > > >   %13 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 1
> > > > >   store ptr %5, ptr %13, align 8
> > > > >   %14 = getelementptr inbounds [4 x i64], ptr %4, i32 0,

[PATCH] D145883: [Flang][RISCV] Emit target features for RISC-V

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added inline comments.



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:112-114
+  case llvm::Triple::riscv64:
+getTargetFeatures(D, Triple, Args, CmdArgs, /*ForAs*/ false);
+break;

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.



Comment at: flang/test/Driver/target-cpu-features.f90:25-26
 
+! RUN: %flang --target=riscv32-linux-gnu -c %s -### 2>&1 \
+! RUN: | FileCheck %s -check-prefix=CHECK-RV32
+

32-bit architectures are generally not supported. This test could be misleading.


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

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1357
+/// Process MapOperands for Target Data directives.
+static LogicalResult processMapOperand(
+llvm::IRBuilderBase &builder, LLVM::ModuleTranslation &moduleTranslation,

TIFitis wrote:
> TIFitis wrote:
> > kiranchandramohan wrote:
> > > TIFitis wrote:
> > > > kiranchandramohan wrote:
> > > > > TIFitis wrote:
> > > > > > kiranchandramohan wrote:
> > > > > > > Isn't it possible to sink this whole function into the 
> > > > > > > OpenMPIRBuilder by passing it a list of `mapOpValue` and 
> > > > > > > `mapTypeFlags`?
> > > > > > > `lvm::Value *mapOpValue = moduleTranslation.lookupValue(mapOp);`
> > > > > > > 
> > > > > > > Did i miss something? Or is this in anticipation of more 
> > > > > > > processing required for other types?
> > > > > > I'm not fully sure but we might need more MLIR related things when 
> > > > > > supporting types other than LLVMPointerType. Also there is a call 
> > > > > > to mlir::LLVM::createMappingInformation.
> > > > > > 
> > > > > > I guess it might still be possible to move most of it to the 
> > > > > > IRBuilder, would you like me to do that?
> > > > > Callbacks are useful when there is frontend-specific handling that is 
> > > > > required. If more types require to be handled then it is better to 
> > > > > have the callback. We can revisit this after all types are handled. I 
> > > > > assume, the current handling is for scalars and arrays of known-size.
> > > > I am a novice at FORTRAN so I'm not aware of all  the types and 
> > > > scenarios.
> > > > 
> > > > I've tested the following cases and they work end-to-end:
> > > > 
> > > > **Fortran:**
> > > > ```
> > > > subroutine openmp_target_data_region(a)
> > > > real :: a(*)
> > > > integer :: b(1024)
> > > > character :: c
> > > > integer, pointer :: p
> > > > !$omp target enter data map(to: a, b, c, p)
> > > > end subroutine openmp_target_data_region
> > > > ```
> > > > 
> > > > **LLVM IR(** `flang-new -fc1 -emit-llvm -fopenmp test.f90 -o test.ll`** 
> > > > ):**
> > > > 
> > > > ```
> > > > ; ModuleID = 'FIRModule'
> > > > source_filename = "FIRModule"
> > > > target datalayout = 
> > > > "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
> > > > target triple = "x86_64-unknown-linux-gnu"
> > > > 
> > > > %struct.ident_t = type { i32, i32, i32, i32, ptr }
> > > > 
> > > > @0 = private unnamed_addr constant [13 x i8] c"loc(unknown)\00", align 1
> > > > @1 = private unnamed_addr constant [56 x i8] 
> > > > c";/home/akash/Documents/scratch/test2.f90;unknown;3;16;;\00", align 1
> > > > @2 = private unnamed_addr constant [56 x i8] 
> > > > c";/home/akash/Documents/scratch/test2.f90;unknown;4;18;;\00", align 1
> > > > @3 = private unnamed_addr constant [56 x i8] 
> > > > c";/home/akash/Documents/scratch/test2.f90;unknown;5;25;;\00", align 1
> > > > @4 = private unnamed_addr constant [23 x i8] 
> > > > c";unknown;unknown;0;0;;\00", align 1
> > > > @5 = private unnamed_addr constant %struct.ident_t { i32 0, i32 2, i32 
> > > > 0, i32 22, ptr @4 }, align 8
> > > > @.offload_maptypes = private unnamed_addr constant [4 x i64] [i64 1, 
> > > > i64 1, i64 1, i64 1]
> > > > @.offload_mapnames = private constant [4 x ptr] [ptr @0, ptr @1, ptr 
> > > > @2, ptr @3]
> > > > 
> > > > declare ptr @malloc(i64)
> > > > 
> > > > declare void @free(ptr)
> > > > 
> > > > define void @openmp_target_data_region_(ptr %0) {
> > > >   %2 = alloca [4 x ptr], align 8
> > > >   %3 = alloca [4 x ptr], align 8
> > > >   %4 = alloca [4 x i64], align 8
> > > >   %5 = alloca [1024 x i32], i64 1, align 4
> > > >   %6 = alloca [1 x i8], i64 1, align 1
> > > >   %7 = alloca { ptr, i64, i32, i8, i8, i8, i8 }, i64 1, align 8
> > > >   %8 = alloca ptr, i64 1, align 8
> > > >   store ptr null, ptr %8, align 8
> > > >   br label %entry
> > > > 
> > > > entry:; preds = %1
> > > >   %9 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 0
> > > >   store ptr %0, ptr %9, align 8
> > > >   %10 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 0
> > > >   store ptr %0, ptr %10, align 8
> > > >   %11 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 0
> > > >   store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), 
> > > > ptr %11, align 8
> > > >   %12 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 1
> > > >   store ptr %5, ptr %12, align 8
> > > >   %13 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 1
> > > >   store ptr %5, ptr %13, align 8
> > > >   %14 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 1
> > > >   store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), 
> > > > ptr %14, align 8
> > > >   %15 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 2
> > > >   store ptr %6, ptr %15, align 8
> > > >   %16 = getelementptr inbounds [4 x ptr], ptr 

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan requested changes to this revision.
kiranchandramohan added inline comments.
This revision now requires changes to proceed.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1357
+/// Process MapOperands for Target Data directives.
+static LogicalResult processMapOperand(
+llvm::IRBuilderBase &builder, LLVM::ModuleTranslation &moduleTranslation,

TIFitis wrote:
> kiranchandramohan wrote:
> > TIFitis wrote:
> > > kiranchandramohan wrote:
> > > > Isn't it possible to sink this whole function into the OpenMPIRBuilder 
> > > > by passing it a list of `mapOpValue` and `mapTypeFlags`?
> > > > `lvm::Value *mapOpValue = moduleTranslation.lookupValue(mapOp);`
> > > > 
> > > > Did i miss something? Or is this in anticipation of more processing 
> > > > required for other types?
> > > I'm not fully sure but we might need more MLIR related things when 
> > > supporting types other than LLVMPointerType. Also there is a call to 
> > > mlir::LLVM::createMappingInformation.
> > > 
> > > I guess it might still be possible to move most of it to the IRBuilder, 
> > > would you like me to do that?
> > Callbacks are useful when there is frontend-specific handling that is 
> > required. If more types require to be handled then it is better to have the 
> > callback. We can revisit this after all types are handled. I assume, the 
> > current handling is for scalars and arrays of known-size.
> I am a novice at FORTRAN so I'm not aware of all  the types and scenarios.
> 
> I've tested the following cases and they work end-to-end:
> 
> **Fortran:**
> ```
> subroutine openmp_target_data_region(a)
> real :: a(*)
> integer :: b(1024)
> character :: c
> integer, pointer :: p
> !$omp target enter data map(to: a, b, c, p)
> end subroutine openmp_target_data_region
> ```
> 
> **LLVM IR(** `flang-new -fc1 -emit-llvm -fopenmp test.f90 -o test.ll`** ):**
> 
> ```
> ; ModuleID = 'FIRModule'
> source_filename = "FIRModule"
> target datalayout = 
> "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
> target triple = "x86_64-unknown-linux-gnu"
> 
> %struct.ident_t = type { i32, i32, i32, i32, ptr }
> 
> @0 = private unnamed_addr constant [13 x i8] c"loc(unknown)\00", align 1
> @1 = private unnamed_addr constant [56 x i8] 
> c";/home/akash/Documents/scratch/test2.f90;unknown;3;16;;\00", align 1
> @2 = private unnamed_addr constant [56 x i8] 
> c";/home/akash/Documents/scratch/test2.f90;unknown;4;18;;\00", align 1
> @3 = private unnamed_addr constant [56 x i8] 
> c";/home/akash/Documents/scratch/test2.f90;unknown;5;25;;\00", align 1
> @4 = private unnamed_addr constant [23 x i8] c";unknown;unknown;0;0;;\00", 
> align 1
> @5 = private unnamed_addr constant %struct.ident_t { i32 0, i32 2, i32 0, i32 
> 22, ptr @4 }, align 8
> @.offload_maptypes = private unnamed_addr constant [4 x i64] [i64 1, i64 1, 
> i64 1, i64 1]
> @.offload_mapnames = private constant [4 x ptr] [ptr @0, ptr @1, ptr @2, ptr 
> @3]
> 
> declare ptr @malloc(i64)
> 
> declare void @free(ptr)
> 
> define void @openmp_target_data_region_(ptr %0) {
>   %2 = alloca [4 x ptr], align 8
>   %3 = alloca [4 x ptr], align 8
>   %4 = alloca [4 x i64], align 8
>   %5 = alloca [1024 x i32], i64 1, align 4
>   %6 = alloca [1 x i8], i64 1, align 1
>   %7 = alloca { ptr, i64, i32, i8, i8, i8, i8 }, i64 1, align 8
>   %8 = alloca ptr, i64 1, align 8
>   store ptr null, ptr %8, align 8
>   br label %entry
> 
> entry:; preds = %1
>   %9 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 0
>   store ptr %0, ptr %9, align 8
>   %10 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 0
>   store ptr %0, ptr %10, align 8
>   %11 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 0
>   store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr 
> %11, align 8
>   %12 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 1
>   store ptr %5, ptr %12, align 8
>   %13 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 1
>   store ptr %5, ptr %13, align 8
>   %14 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 1
>   store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr 
> %14, align 8
>   %15 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 2
>   store ptr %6, ptr %15, align 8
>   %16 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 2
>   store ptr %6, ptr %16, align 8
>   %17 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 2
>   store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr 
> %17, align 8
>   %18 = getelementptr inbounds [4 x ptr], ptr %2, i32 0, i32 3
>   store ptr %7, ptr %18, align 8
>   %19 = getelementptr inbounds [4 x ptr], ptr %3, i32 0, i32 3
>   store ptr %7, ptr %19, align 8
>   %20 = getelementptr inbounds [4 x i64], ptr %4, i32 0, i32 3
>   store i64 ptrtoint (ptr getelementptr (ptr, ptr null, i32 1) to i64), ptr 
> %20, a

[PATCH] D144864: [Flang][Driver][MLIR] Add -fopenmp-is-device to Flang and link to an omp.is_device attribute

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan accepted this revision.
kiranchandramohan added a comment.

LG.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144864

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

[PATCH] D144864: [Flang][Driver][MLIR] Add -fopenmp-is-device to Flang and link to an omp.is_device attribute

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

In D144864#4175332 , @agozillon wrote:

> In D144864#4175257 , 
> @kiranchandramohan wrote:
>
>> LG. See one minor comment in the tests.
>>
>> I would prefer having an Interface for Target Modules if that could be made 
>> to work. I guess this can be taken up separately after 
>> https://reviews.llvm.org/D144883.
>
> Thank you Kiran, by interface for Target Modules do you mean a new omp.module 
> or this type of external interface: 
> https://mlir.llvm.org/docs/Interfaces/#external-models-for-attribute-operation-and-type-interfaces
>
> And quite possibly, @skatrak is currently looking into the review comments in 
> the referenced patch (as he found similar use for the patch), so we shall see 
> where the results lead to. Never opposed to improving on the infrastructure 
> and this is something we will iterate on as we progress with offloading! :)

The latter one (external interface).




Comment at: flang/test/Lower/OpenMP/omp-is-device.f90:8-17
+!DEVICE: module attributes {{{.*}}, omp.is_device = true{{.*}}}
+!HOST: module attributes {{{.*}}, omp.is_device = false{{.*}}}
+!DEVICE-FLAG-ONLY: module attributes {{{.*}}"
+!DEVICE-FLAG-ONLY-NOT: , omp.is_device = {{.*}}
+!DEVICE-FLAG-ONLY-SAME: } 
+!BBC-DEVICE: module attributes {{{.*}}, omp.is_device = true{{.*}}}
+!BBC-HOST: module attributes {{{.*}}, omp.is_device = false{{.*}}}

agozillon wrote:
> kiranchandramohan wrote:
> > Any reason to have separate checks for essentially the same (e.g: DEVICE vs 
> > BBC-DEVICE)?
> In this case to test the addition of the flag to bbc and also to the Flang 
> driver, and testing the flag has the same behavior in both. There is perhaps 
> a way to make the test simpler that I'm unaware of though? e.g. a way to 
> merge DEVICE/BBC-DEVICE into one check
If DEVICE and BBC-DEVICE are checking the same things, then you only need to 
have one of them (say DEVICE) and use it in the check-prefix of both.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144864

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

[PATCH] D144864: [Flang][Driver][MLIR] Add -fopenmp-is-device to Flang and link to an omp.is_device attribute

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan accepted this revision.
kiranchandramohan added a comment.

LG. See one minor comment in the tests.

I would prefer having an Interface for Target Modules if that could be made to 
work. I guess this can be taken up separately after 
https://reviews.llvm.org/D144883.




Comment at: flang/test/Lower/OpenMP/omp-is-device.f90:8-17
+!DEVICE: module attributes {{{.*}}, omp.is_device = true{{.*}}}
+!HOST: module attributes {{{.*}}, omp.is_device = false{{.*}}}
+!DEVICE-FLAG-ONLY: module attributes {{{.*}}"
+!DEVICE-FLAG-ONLY-NOT: , omp.is_device = {{.*}}
+!DEVICE-FLAG-ONLY-SAME: } 
+!BBC-DEVICE: module attributes {{{.*}}, omp.is_device = true{{.*}}}
+!BBC-HOST: module attributes {{{.*}}, omp.is_device = false{{.*}}}

Any reason to have separate checks for essentially the same (e.g: DEVICE vs 
BBC-DEVICE)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144864

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

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

Thanks for the update and the replies. See comments inline.




Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1561
+  /// should be placed.
+  /// \param HasRegion True if the op has a region associated with it, false
+  /// otherwise

TIFitis wrote:
> kiranchandramohan wrote:
> > Can't the presence/absence of the BodyGenCB indicate the presence/absence 
> > of a region?
> The `BodyGenCB` is always present as an argument right? Passing a `nullptr` 
> when its not required doesn't seem possible at least when using the 
> `BodyGenCallbackTy`. Is there a way to selectively pass the `BodyGenCB`?
The only optional CallBack seems to be the `FinalizeCallbackTy`. It is defined 
as an `std::function` whereas `BodyGenCallBackTy` is defined as a 
`function_ref`. But the definition of `function_ref` looks like it can be used 
to check whether it is a `nullptr` or not. Did you face any issues in trying to 
make it optional with a default parameter value of `nullptr`?

https://llvm.org/doxygen/STLFunctionalExtras_8h_source.html#l00036

```
  void emitCancelationCheckImpl(Value *CancelFlag,
omp::Directive CanceledDirective,
FinalizeCallbackTy ExitCB = {});
```



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4052-4060
+  // LLVM utilities like blocks with terminators.
+  auto *UI = Builder.CreateUnreachable();
+  Instruction *ThenTI = UI, *ElseTI = nullptr;
+  if (IfCond) {
+SplitBlockAndInsertIfThenElse(IfCond, UI, &ThenTI, &ElseTI);
+ThenTI->getParent()->setName("omp_if.then");
+ElseTI->getParent()->setName("omp_if.else");

TIFitis wrote:
> kiranchandramohan wrote:
> > TIFitis wrote:
> > > kiranchandramohan wrote:
> > > > There is some recent recommendation to not insert artificial 
> > > > instructions and remove them. The preferred approach is to use 
> > > > `splitBB` function(s) inside the OpenMPIRBuilder. This function works 
> > > > on blocks without terminators. You can consult the `IfCondition` code 
> > > > in `createParallel`.
> > > Thanks a lot for taking the time to review this lengthy patch.
> > > 
> > > This one seems a bit tricky to do. At first glance `createParallel` seems 
> > > to be doing something different where its calling different runtime 
> > > functions based on the `IfCondition` instead of much in the way of 
> > > Control Flow changes.
> > > 
> > > The `unreachable` inst helps out a lot here as it makes it really easy to 
> > > keep trace of the resume point for adding instructions after the 
> > > `BodyGen` codes are generated.
> > > 
> > > I am still looking into finding a way to do this elegantly without having 
> > > to use the `unreachable` instruction, but would it be a major blocker if 
> > > we had to keep it?
> > > 
> > > I have addressed all the other changes you requested.
> > Thanks for explaining the need for the `unreachable`.  I will leave it with 
> > you on whether to make the change.
> > 
> > You can see an instance of a past request for not using temporary 
> > instruction. That patch (if for createTask) addressed the issue one way.
> > https://reviews.llvm.org/D130615#inline-1257711
> > 
> > I gave a quick try and came up with the following code. I don't think it is 
> > very elegant, but it is one way to do it. 
> > ```
> > -  // LLVM utilities like blocks with terminators.
> > -  auto *UI = Builder.CreateUnreachable();
> > +  BasicBlock *ContBB = nullptr;
> >if (IfCond) {
> > -auto *ThenTI =
> > -SplitBlockAndInsertIfThen(IfCond, UI, /* Unreachable */ false);
> > -ThenTI->getParent()->setName("omp_if.then");
> > -Builder.SetInsertPoint(ThenTI);
> > -  } else {
> > -Builder.SetInsertPoint(UI);
> > +BasicBlock *EntryBB = Builder.GetInsertBlock();
> > +ContBB = splitBB(Builder, /*CreateBranch*/ false);
> > +BasicBlock *ThenBB =
> > +BasicBlock::Create(Builder.getContext(), EntryBB->getName() + ".if",
> > +   ContBB->getParent(), ContBB);
> > +ContBB->setName(EntryBB->getName() + ".if.end");
> > +Builder.CreateCondBr(IfCond, ThenBB, ContBB);
> > +Builder.SetInsertPoint(ThenBB);
> > +Builder.CreateBr(ContBB);
> > +Builder.SetInsertPoint(ThenBB->getTerminator());
> >}
> >  
> >ProcessMapOpCB(Builder.saveIP(), Builder.saveIP());
> > @@ -4087,9 +4090,10 @@ OpenMPIRBuilder::InsertPointTy 
> > OpenMPIRBuilder::createTargetData(
> >  emitMapperCall(Builder.saveIP(), beginMapperFunc, srcLocInfo, 
> > MapTypesArg,
> > MapNamesArg, MapperAllocas, DeviceID, 
> > MapTypeFlags.size());
> >  
> > +BasicBlock *DataExitBB = splitBB(Builder, true, "target_data.exit");
> >  BodyGenCB(Builder.saveIP(), Builder.saveIP());
> >  
> > -Builder.SetInsertPoint(UI->getParent());
> > +Builder.SetInsertPoint(DataExitBB, DataExitBB->begin());
> >  // Create call to end the

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan requested changes to this revision.
kiranchandramohan added a comment.
This revision now requires changes to proceed.

I have some more comments.




Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1556
 
+  /// Generator for '#omp target data'
+  ///

Nit: In the following comments either use full stops for all.

It will be helpful if the comments below refer to the OpenMP construct or 
clauses rather than the MLIR representation. Forr e.g Map Clause instead of Map 
operand.



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1561
+  /// should be placed.
+  /// \param HasRegion True if the op has a region associated with it, false
+  /// otherwise

Can't the presence/absence of the BodyGenCB indicate the presence/absence of a 
region?



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1567
+  /// \param MapperAllocas Pointers to the AllocInsts for the map clause
+  /// \param MapperFunc Mapper Fucntion to be called for the Target Data op
+  /// \param DeviceID Stores the DeviceID from the device clause

This should be expanded to say whether it means `begin` or `end` mapper based 
on the value of the boolean and probably also renamed to `isBegin` or `isEnter` 
or something like that.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4052-4060
+  // LLVM utilities like blocks with terminators.
+  auto *UI = Builder.CreateUnreachable();
+  Instruction *ThenTI = UI, *ElseTI = nullptr;
+  if (IfCond) {
+SplitBlockAndInsertIfThenElse(IfCond, UI, &ThenTI, &ElseTI);
+ThenTI->getParent()->setName("omp_if.then");
+ElseTI->getParent()->setName("omp_if.else");

TIFitis wrote:
> kiranchandramohan wrote:
> > There is some recent recommendation to not insert artificial instructions 
> > and remove them. The preferred approach is to use `splitBB` function(s) 
> > inside the OpenMPIRBuilder. This function works on blocks without 
> > terminators. You can consult the `IfCondition` code in `createParallel`.
> Thanks a lot for taking the time to review this lengthy patch.
> 
> This one seems a bit tricky to do. At first glance `createParallel` seems to 
> be doing something different where its calling different runtime functions 
> based on the `IfCondition` instead of much in the way of Control Flow changes.
> 
> The `unreachable` inst helps out a lot here as it makes it really easy to 
> keep trace of the resume point for adding instructions after the `BodyGen` 
> codes are generated.
> 
> I am still looking into finding a way to do this elegantly without having to 
> use the `unreachable` instruction, but would it be a major blocker if we had 
> to keep it?
> 
> I have addressed all the other changes you requested.
Thanks for explaining the need for the `unreachable`.  I will leave it with you 
on whether to make the change.

You can see an instance of a past request for not using temporary instruction. 
That patch (if for createTask) addressed the issue one way.
https://reviews.llvm.org/D130615#inline-1257711

I gave a quick try and came up with the following code. I don't think it is 
very elegant, but it is one way to do it. 
```
-  // LLVM utilities like blocks with terminators.
-  auto *UI = Builder.CreateUnreachable();
+  BasicBlock *ContBB = nullptr;
   if (IfCond) {
-auto *ThenTI =
-SplitBlockAndInsertIfThen(IfCond, UI, /* Unreachable */ false);
-ThenTI->getParent()->setName("omp_if.then");
-Builder.SetInsertPoint(ThenTI);
-  } else {
-Builder.SetInsertPoint(UI);
+BasicBlock *EntryBB = Builder.GetInsertBlock();
+ContBB = splitBB(Builder, /*CreateBranch*/ false);
+BasicBlock *ThenBB =
+BasicBlock::Create(Builder.getContext(), EntryBB->getName() + ".if",
+   ContBB->getParent(), ContBB);
+ContBB->setName(EntryBB->getName() + ".if.end");
+Builder.CreateCondBr(IfCond, ThenBB, ContBB);
+Builder.SetInsertPoint(ThenBB);
+Builder.CreateBr(ContBB);
+Builder.SetInsertPoint(ThenBB->getTerminator());
   }
 
   ProcessMapOpCB(Builder.saveIP(), Builder.saveIP());
@@ -4087,9 +4090,10 @@ OpenMPIRBuilder::InsertPointTy 
OpenMPIRBuilder::createTargetData(
 emitMapperCall(Builder.saveIP(), beginMapperFunc, srcLocInfo, MapTypesArg,
MapNamesArg, MapperAllocas, DeviceID, MapTypeFlags.size());
 
+BasicBlock *DataExitBB = splitBB(Builder, true, "target_data.exit");
 BodyGenCB(Builder.saveIP(), Builder.saveIP());
 
-Builder.SetInsertPoint(UI->getParent());
+Builder.SetInsertPoint(DataExitBB, DataExitBB->begin());
 // Create call to end the data region.
 emitMapperCall(Builder.saveIP(), endMapperFunc, srcLocInfo, MapTypesArg,
MapNamesArg, MapperAllocas, DeviceID, MapTypeFlags.size());
@@ -4100,11 +4104,9 @@ OpenMPIRBuilder::InsertPointTy 
OpenMPIRBuilder::createTargetData(

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan requested changes to this revision.
kiranchandramohan added a comment.
This revision now requires changes to proceed.

Thanks for making the change.
I am still going through the patch, I have a few comments.




Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1574-1575
+  const LocationDescription &Loc, OpenMPIRBuilder::InsertPointTy CodeGenIP,
+  bool HasRegion, SmallVector &MapTypeFlags,
+  SmallVector &MapNames, struct MapperAllocas &MapperAllocas,
+  Function *MapperFunc, int64_t DeviceID, Value *IfCond,

SmallVector -> SmallVectorImpl 
https://llvm.org/docs/ProgrammersManual.html#llvm-adt-smallvector-h



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4052-4060
+  // LLVM utilities like blocks with terminators.
+  auto *UI = Builder.CreateUnreachable();
+  Instruction *ThenTI = UI, *ElseTI = nullptr;
+  if (IfCond) {
+SplitBlockAndInsertIfThenElse(IfCond, UI, &ThenTI, &ElseTI);
+ThenTI->getParent()->setName("omp_if.then");
+ElseTI->getParent()->setName("omp_if.else");

There is some recent recommendation to not insert artificial instructions and 
remove them. The preferred approach is to use `splitBB` function(s) inside the 
OpenMPIRBuilder. This function works on blocks without terminators. You can 
consult the `IfCondition` code in `createParallel`.



Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:4983
+  CallInst *TargetDataCall = dyn_cast(&BB->back());
+  BB->dump();
+  EXPECT_NE(TargetDataCall, nullptr);

Is this a debugging leftover?



Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:4990
+  EXPECT_TRUE(TargetDataCall->getOperand(2)->getType()->isIntegerTy(32));
+  EXPECT_TRUE(TargetDataCall->getOperand(8)->getType()->isPointerTy());
+}

Call verifyModule to ensure the IR is well formed.



Comment at: mlir/include/mlir/Target/LLVMIR/Dialect/OpenMPCommon.h:13-14
+
+#ifndef MLIR_TARGET_LLVMIR_DIALECT_UTILS_H
+#define MLIR_TARGET_LLVMIR_DIALECT_UTILS_H
+

Nit: The header guards would need changing.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1471
+mapTypes = enterDataOp.getMapTypes();
+mapperFunc = ompBuilder->getOrCreateRuntimeFunctionPtr(
+llvm::omp::OMPRTL___tgt_target_data_begin_mapper);

Ideally we would not want to create an OpenMP runtime calls in the Translation. 
This is the job of the OpenMPIRbuilder. I would recommend passing a boolean to 
the OpenMPIRBuilder and let the IRBuilder pick up the runtime function.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1492
+mapTypes = exitDataOp.getMapTypes();
+mapperFunc = ompBuilder->getOrCreateRuntimeFunctionPtr(
+llvm::omp::OMPRTL___tgt_target_data_end_mapper);

Same comment as above.



Comment at: mlir/test/Target/LLVMIR/omptarget-llvm.mlir:1
+// RUN: mlir-translate -mlir-to-llvmir -split-input-file %s | FileCheck %s
+

In MLIR it is preferred to test the minimal set of functionalities. So you will 
have to minimize the CHECK lines and write them manually. Focus on Checking the 
runtime call and any important IR inserted by the IRBuilder. Keep the contents 
of the region to a minimum.
https://mlir.llvm.org/getting_started/TestingGuide/#filecheck-best-practices


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

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

[PATCH] D144864: [Flang][Driver][MLIR] Add -fopenmp-is-device to Flang and link to an omp.is_device attribute

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

Could you update the discourse thread 
(https://discourse.llvm.org/t/rfc-omp-module-and-omp-function-vs-dialect-attributes-to-encode-openmp-properties/67998)
 with the chosen approach and reasons before proceeding?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144864

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

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

In D142914#4145224 , @TIFitis wrote:

> In D142914#4144475 , 
> @kiranchandramohan wrote:
>
>> Please add tests for the MLIR portion.
>
> Can you please tell me where to add these?

In `mlir/test/Target/LLVMIR/openmp-llvm.mlir`. You can also consider starting a 
new file for target constructs in that directory.




Comment at: mlir/lib/Target/LLVMIR/Dialect/Utils.cpp:1
+//===- Utils.cpp - General Utils for translating MLIR dialect to LLVM 
IR---===//
+//

TIFitis wrote:
> kiranchandramohan wrote:
> > Nit: I would prefer the OpenMPCommon.cpp name suggested in 
> > https://discourse.llvm.org/t/rfc-adding-new-util-file-to-mlir-dialect-translation/68221.
> >  
> Would you also like me to move the file inside OpenMP ( 
> `mlir/lib/Target/LLVMIR/Dialect/OpenMP` ) ?
No, the current location is fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

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

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan requested changes to this revision.
kiranchandramohan added a comment.
This revision now requires changes to proceed.

Please add tests for the MLIR portion.

Could you also post the full LLVM IR for a construct with the map clause?




Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:4157
+Value *OpenMPIRBuilder::getSizeInBytes(Value *BasePtr) {
+
+  LLVMContext &Ctx = Builder.getContext();

Nit:Remove empty line.



Comment at: mlir/lib/Target/LLVMIR/Dialect/Utils.cpp:1
+//===- Utils.cpp - General Utils for translating MLIR dialect to LLVM 
IR---===//
+//

Nit: I would prefer the OpenMPCommon.cpp name suggested in 
https://discourse.llvm.org/t/rfc-adding-new-util-file-to-mlir-dialect-translation/68221.
 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

___
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

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

FWIW, We have a plugin 
(https://github.com/llvm/llvm-project/tree/main/flang/examples/FlangOmpReport) 
that goes over the parse-tree and reports all the OpenMP constructs and clauses 
seen.


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] D141910: [OpenMP][OMPIRBuilder]Move SIMD alignment calculation to LLVM Frontend

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan accepted this revision.
kiranchandramohan added a comment.

LG.


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

https://reviews.llvm.org/D141910

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

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

Please add more description to the summary regarding the scope of this patch. 
If it is only able to lower specific llvm-dialect types, please call out that. 
Please also explain the split in work between mlir::Translation and 
OpenMPIRbuilder. You can also consider splitting this into two patches. One 
that just adds the changes to OpenMPIRBuilder and the child version of the 
patch adding the translation in MLIR.




Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1342-1382
+/// Create a constant string location from the MLIR Location information.
+static llvm::Constant *
+createSourceLocStrFromLocation(Location loc, llvm::OpenMPIRBuilder &builder,
+   StringRef name, uint32_t &strLen) {
+  if (auto fileLoc = loc.dyn_cast()) {
+StringRef fileName = fileLoc.getFilename();
+unsigned lineNo = fileLoc.getLine();

TIFitis wrote:
> clementval wrote:
> > TIFitis wrote:
> > > clementval wrote:
> > > > TIFitis wrote:
> > > > > clementval wrote:
> > > > > > TIFitis wrote:
> > > > > > > clementval wrote:
> > > > > > > > TIFitis wrote:
> > > > > > > > > clementval wrote:
> > > > > > > > > > kiranchandramohan wrote:
> > > > > > > > > > > clementval wrote:
> > > > > > > > > > > > Instead of copy pasting this from 
> > > > > > > > > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp`
> > > > > > > > > > > >  can you extract it and put it in a common shared file 
> > > > > > > > > > > > so bith translation can use the same code without 
> > > > > > > > > > > > duplication?
> > > > > > > > > > > @raghavendhra put up a patch some time back and he faced 
> > > > > > > > > > > some issues. It might be good to check with him or may be 
> > > > > > > > > > > he can comment here.
> > > > > > > > > > > https://reviews.llvm.org/D127037
> > > > > > > > > > > https://discourse.llvm.org/t/rfc-for-refactoring-common-code-for-openacc-and-openmp/63833
> > > > > > > > > > Just moving the three functions should be trivial. I'm not 
> > > > > > > > > > talking about the processMapOperand.
> > > > > > > > > I've moved `getSizeInBytes`.
> > > > > > > > > 
> > > > > > > > > The other two functions make use of `mlir::Location` and thus 
> > > > > > > > > can't be moved trivially.
> > > > > > > > > 
> > > > > > > > > I can still try to move them by individually passing the 
> > > > > > > > > elements of `mlir::Location` but that might not be ideal. Is 
> > > > > > > > > that what you'd like?
> > > > > > > > What about a new header file in 
> > > > > > > > `mlir/include/mlir/Target/LLVMIR/Dialect/**` shared by 
> > > > > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp`
> > > > > > > >  and 
> > > > > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp`.
> > > > > > > >  That should be doable. 
> > > > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp`
> > > > > > >  and 
> > > > > > > `mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp`
> > > > > > >  already have access to the common `mlir::Location` type.
> > > > > > > 
> > > > > > > Problem is that `OMPIRBuilder.cpp` is the only common file 
> > > > > > > between them  where I can move the two functions to. Currently 
> > > > > > > there are no `mlir/**` include files in `OMPIRBuilder.cpp` and it 
> > > > > > > seems to me like a strict design choice to have it that way.
> > > > > > The functions can be header only. Why do you need to put them in 
> > > > > > the OMPIRBuilder.cpp? I think it is better than duplicate the exact 
> > > > > > same code over. 
> > > > > Sorry, I misunderstood you earlier.
> > > > > I've added a new header file 
> > > > > `mlir/include/mlir/Target/LLVMIR/Dialect/Utils.h`, this is my first 
> > > > > attempt at adding a new header file, please let me know if you find 
> > > > > any issues.
> > > > Thanks! That's what I had in mind. We might want to check with MLIR 
> > > > folks if `mlir::utils` is suited for that. I don't mind if it is 
> > > > `mlir::omp::builder` or smth similar since it is related to the 
> > > > OMPIRBuilder.
> > > Since the utils file is common to all the dialects I kept it as 
> > > `mlir::utils`.
> > > 
> > > How do I get the opinion from people working in MLIR on this, can you 
> > > suggest some reviewers whom I can add?
> > It's only valid for translation to the `llvmir` dialect so that why 
> > `mlir::utils` seems to generic to me. 
> > 
> > Maybe @ftynse has some thoughts on this. 
> I agree with you on that, would perhaps renaming it to something like 
> `mlir::dialect-utils` be a better option?
You can post in MLIR discourse MLIR section 
(https://discourse.llvm.org/c/mlir/31) to get an opinion.

open-directive-utils , ompbuilder-utils are other options. Simialr names could 
be considered for the file name as well.






Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST 

[PATCH] D140415: [flang] stack arrays pass

[Kiran Chandramohan via Phabricator via cfe-commits]

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

LGTM. Please wait till end of day Monday before you submit to provide other 
reviewers with a chance to provide further comments or request changes.

You can consider inlining D141401  in this 
pass till it is merged.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140415

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

[PATCH] D140415: [flang] stack arrays pass

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

Looks OK. I have a few questions and some minor comments inline. It might be 
good to pull in a bit of info from the RFC into the Summary, particularly 
saying why a dataflow analysis is necessary, what operations are involved in 
the analysis etc.

Could we have used the Dominance and PostDominance information to find out the 
Allocs and Frees that could have been replaced? I saw the following functions 
for individual Ops but not for the case where a set of ops dominates or 
post-dominates. So may be not with the existing infra.

  bool DominanceInfo::properlyDominatesImpl(Operation *a, Operation *b
  bool PostDominanceInfo::properlyPostDominates(Operation *a, Operation *b) {

I guess, we are not capturing the following because of different values.

  module {
func.func @dfa2(%arg0: i1) {
  cf.cond_br %arg0, ^bb1, ^bb2
^bb1:  // pred: ^bb0
  %a = fir.allocmem !fir.array<1xi8>
  cf.br ^bb3(%a : !fir.heap>)
^bb2:  // pred: ^bb0
  %b = fir.allocmem !fir.array<1xi8>
  cf.br ^bb3(%b : !fir.heap>)
^bb3(%0: !fir.heap>):  // 2 preds: ^bb1, ^bb2
  fir.freemem %0 : !fir.heap>
  return
}
  }




Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:436-438
+if (lattice) {
+  point.join(*lattice);
+}

Nit: No brace here



Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:442
+  point.appendFreedValues(freedValues);
+
+  for (mlir::Value freedValue : freedValues) {

A comment here would be useful on why we need to look at the freed values only.



Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:482-486
+  for (mlir::Operation *user : allocmem.getOperation()->getUsers()) {
+if (mlir::isa(user)) {
+  rewriter.eraseOp(user);
+}
+  }

Nit: Braces might not be require here.



Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:532
+
+  // Find when the last operand value becomes available
+  mlir::Block *operandsBlock = nullptr;

Might be worth checking whether we have a function for this in MLIR core.



Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:545-547
+  // Operation::isBeforeInBlock requires the operations to be in the same
+  // block. The best we can do is the location of the allocmem.
+  return checkReturn(oldAlloc.getOperation());

Theoretically speaking, we can use the dominance info to determine whether one 
block dominates the other as well to handle cases like the following where we 
are finding the operands of `func`. But I guess that is probably not required.
```
b1:
x = opA
br b2
b2:
y = opB
br b3
b3:
z = func(x,y)
```




Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:560
+
lastOperand->getParentOfType();
+if (lastOpOmpRegion == oldOmpRegion)
+  return checkReturn(lastOperand);

Do we have a test for this, and in general for the OpenMP handling?



Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:572-574
+  if (oldOmpRegion) {
+return checkReturn(oldOmpRegion.getAllocaBlock());
+  }

Nit: No need for braces here.



Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:593-597
+  for (mlir::Operation *user : oldAllocOp->getUsers()) {
+if (mlir::isa(user)) {
+  freeOps.push_back(user);
+}
+  }

Nit: braces are not required here.



Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:732
+  mlir::applyPartialConversion(func, target, std::move(patterns {
+mlir::emitError(func->getLoc(), "error in stack arrays optimization\n");
+signalPassFailure();

Nit: Is this error usually given in passes?



Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:696-697
+
+void StackArraysPass::runOnOperation() {
+  mlir::ModuleOp mod = getOperation();
+

tblah wrote:
> kiranchandramohan wrote:
> > From the following code, it seems the functions are processed 
> > independently. Can this be a `Function` pass?
> It can't. `fir::factory::getLlvm::getStackSave` and 
> `fir::factory::getLlvmSatckRestore` add function declarations to the 
> module-level. If functions are processed in different threads, there is a 
> race condition when the `fir::builder` first checks to see if the function 
> already exists in the module and if not, adds it.
Not for this patch: May be these can all be preinserted at the beginning of the 
pass pipeline and removed if not used at the end of the pass pipeline?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140415

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bi

[PATCH] D140415: [flang] stack arrays pass

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

Thanks for this patch @tblah. I had a first look. See comments inline. Have not 
gone through the core part in full yet.




Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:433
+  llvm::DenseSet freedValues;
+  func->walk([&](mlir::func::ReturnOp child) {
+const LatticePoint *lattice = solver.lookupState(child);

Do we have a test with multiple returns?



Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:535
+LLVM_DEBUG(llvm::dbgs() << "--considering operand " << operand << "\n");
+mlir::Operation *op = operand.getDefiningOp();
+if (!operandsBlock)

The op might require a check before further use.

See the following test from `arrexp.fir`. (run with `./bin/tco f4.fir`)

```
func.func @f4(%a : !fir.ref>, %b : 
!fir.ref>, %n : index, %m : index, %o : index, %p : index, 
%f : f32) {
  %c1 = arith.constant 1 : index
  %s = fir.shape_shift %o, %n, %p, %m : (index, index, index, index) -> 
!fir.shapeshift<2>
  %vIn = fir.array_load %a(%s) : (!fir.ref>, 
!fir.shapeshift<2>) -> !fir.array
  %wIn = fir.array_load %b(%s) : (!fir.ref>, 
!fir.shapeshift<2>) -> !fir.array
  %r = fir.do_loop %j = %p to %m step %c1 iter_args(%v1 = %vIn) -> 
!fir.array {
%r = fir.do_loop %i = %o to %n step %c1 iter_args(%v = %v1) -> 
!fir.array {
  %x2 = fir.array_fetch %vIn, %i, %j : (!fir.array, index, index) 
-> f32
  %x = fir.array_fetch %wIn, %i, %j : (!fir.array, index, index) 
-> f32
  %y = arith.addf %x, %f : f32
  %y2 = arith.addf %y, %x2 : f32
  %i2 = arith.addi %i, %c1 : index
  %r = fir.array_update %v, %y2, %i2, %j : (!fir.array, f32, 
index, index) -> !fir.array
  fir.result %r : !fir.array
} 
fir.result %r : !fir.array
  }
  fir.array_merge_store %vIn, %r to %a : !fir.array, 
!fir.array, !fir.ref>
  return
}
```



Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:620-622
+  if (it == candidateOps.end()) {
+return {};
+  }

Nit: Braces not required.



Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:630-638
+  mlir::Location loc = oldAlloc.getLoc();
+  mlir::Type varTy = oldAlloc.getInType();
+  auto unpackName = [](std::optional opt) -> llvm::StringRef {
+if (opt)
+  return *opt;
+return {};
+  };

Nit: Move all these close to the creation of the  `fir:AllocaOp`.



Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:640-641
+
+  if (mlir::Operation *op = insertionPoint.tryGetOperation())
+rewriter.setInsertionPointAfter(op);
+  else {

kiranchandramohan wrote:
> Nit: Use braces here to match `else`.
Nit: Use braces for the `if` block to keep it uniform with the `else` block.




Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:640-642
+  if (mlir::Operation *op = insertionPoint.tryGetOperation())
+rewriter.setInsertionPointAfter(op);
+  else {

Nit: Use braces here to match `else`.



Comment at: flang/lib/Optimizer/Transforms/StackArrays.cpp:696-697
+
+void StackArraysPass::runOnOperation() {
+  mlir::ModuleOp mod = getOperation();
+

From the following code, it seems the functions are processed independently. 
Can this be a `Function` pass?



Comment at: flang/test/Transforms/stack-arrays.f90:136
+! CHECK-SAME: cfgloop
+! CHECK-NEXT: %0 = fir.alloca !fir.array<1xi32>
+! CHECK-NOT: fir.allocmem

Nit: Remove usage of `%0`.



Comment at: flang/test/Transforms/stack-arrays.fir:39-49
+// CHECK:  func.func @dfa1(%arg0: !fir.ref> 
{fir.bindc_name = "cond"}) {
+// CHECK-NEXT:   %c42 = arith.constant 42 : index
+// CHECK-NEXT:   %0 = fir.allocmem !fir.array, %c42 {uniq_name = 
"_QFdfa1Earr.alloc"}
+// CHECK-NEXT:   %1 = fir.load %arg0 : !fir.ref>
+// CHECK-NEXT:   %2 = fir.convert %1 : (!fir.logical<4>) -> i1
+// CHECK-NEXT:   fir.if %2 {
+// CHECK-NEXT: fir.freemem %0 : !fir.heap>

Would it be better to capture the variables and check? At least the allocmem 
and freemem.



Comment at: flang/test/Transforms/stack-arrays.fir:203
+
+// check that stack save/restore are not used when the memalloc and freemem are
+// in different blocks

Is this a case for future improvement?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140415

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

[PATCH] D142914: [MLIR][OpenMP] Added OMPIRBuilder support for Target Data directives.

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a subscriber: raghavendhra.
kiranchandramohan added inline comments.



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:1342-1382
+/// Create a constant string location from the MLIR Location information.
+static llvm::Constant *
+createSourceLocStrFromLocation(Location loc, llvm::OpenMPIRBuilder &builder,
+   StringRef name, uint32_t &strLen) {
+  if (auto fileLoc = loc.dyn_cast()) {
+StringRef fileName = fileLoc.getFilename();
+unsigned lineNo = fileLoc.getLine();

clementval wrote:
> Instead of copy pasting this from 
> `mlir/lib/Target/LLVMIR/Dialect/OpenACC/OpenACCToLLVMIRTranslation.cpp` can 
> you extract it and put it in a common shared file so bith translation can use 
> the same code without duplication?
@raghavendhra put up a patch some time back and he faced some issues. It might 
be good to check with him or may be he can comment here.
https://reviews.llvm.org/D127037
https://discourse.llvm.org/t/rfc-for-refactoring-common-code-for-openacc-and-openmp/63833


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142914

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

[PATCH] D105584: [MLIR][OpenMP] Distribute Construct Operation

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan requested changes to this revision.
kiranchandramohan added a comment.
This revision now requires changes to proceed.

Use `omp.canonical_loop` once it is in.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105584

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

[PATCH] D105584: [MLIR][OpenMP] Distribute Construct Operation

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.
Herald added a subscriber: thopre.

Since the plan is to switch to the Canonical Style operation, may be this can 
wait till the `omp.canonical_loop` changes are in?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105584

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

[PATCH] D142199: [Docs] Replace recommonmark with myst-parser

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

The flang website (flang.llvm.org) or flang.llvm.org/docs are built from these. 
Would you know whether there are changes required are there? 
https://github.com/llvm/llvm-zorg/blob/main/zorg/buildbot/builders/SphinxDocsBuilder.py


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142199

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

[PATCH] D142347: [NFC][Clang] Move DebugOptions to llvm/Frontend for reuse in Flang

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan created this revision.
Herald added a subscriber: sunshaoce.
Herald added a project: All.
kiranchandramohan requested review of this revision.
Herald added subscribers: llvm-commits, cfe-commits, MaskRay.
Herald added projects: clang, LLVM.

Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D142347

Files:
  clang/include/clang/Basic/CodeGenOptions.def
  clang/include/clang/Basic/CodeGenOptions.h
  clang/include/clang/Basic/DebugInfoOptions.h
  clang/include/clang/Driver/ToolChain.h
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/lib/CodeGen/CGOpenMPRuntime.cpp
  clang/lib/CodeGen/CGVTables.cpp
  clang/lib/CodeGen/CodeGenModule.cpp
  clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/lib/Driver/ToolChains/Clang.h
  clang/lib/Driver/ToolChains/Cuda.cpp
  clang/lib/Driver/ToolChains/Cuda.h
  clang/lib/Driver/ToolChains/HIPSPV.cpp
  clang/lib/Driver/ToolChains/HIPSPV.h
  clang/lib/Driver/ToolChains/MSVC.h
  clang/lib/Frontend/CompilerInvocation.cpp
  clang/lib/Frontend/Rewrite/FrontendActions.cpp
  clang/tools/clang-import-test/clang-import-test.cpp
  llvm/include/llvm/Frontend/Debug/Options.h

Index: llvm/include/llvm/Frontend/Debug/Options.h
===
--- llvm/include/llvm/Frontend/Debug/Options.h
+++ llvm/include/llvm/Frontend/Debug/Options.h
@@ -6,10 +6,10 @@
 //
 //===--===//
 
-#ifndef LLVM_CLANG_BASIC_DEBUGINFOOPTIONS_H
-#define LLVM_CLANG_BASIC_DEBUGINFOOPTIONS_H
+#ifndef LLVM_FRONTEND_DEBUG_OPTIONS_H
+#define LLVM_FRONTEND_DEBUG_OPTIONS_H
 
-namespace clang {
+namespace llvm {
 namespace codegenoptions {
 
 enum DebugInfoFormat {
@@ -54,13 +54,9 @@
   UnusedTypeInfo,
 };
 
-enum class DebugTemplateNamesKind {
-  Full,
-  Simple,
-  Mangled
-};
+enum class DebugTemplateNamesKind { Full, Simple, Mangled };
 
 } // end namespace codegenoptions
-} // end namespace clang
+} // end namespace llvm
 
 #endif
Index: clang/tools/clang-import-test/clang-import-test.cpp
===
--- clang/tools/clang-import-test/clang-import-test.cpp
+++ clang/tools/clang-import-test/clang-import-test.cpp
@@ -200,7 +200,7 @@
   Inv->getLangOpts()->CXXExceptions = true;
   // Needed for testing dynamic_cast.
   Inv->getLangOpts()->RTTI = true;
-  Inv->getCodeGenOpts().setDebugInfo(codegenoptions::FullDebugInfo);
+  Inv->getCodeGenOpts().setDebugInfo(llvm::codegenoptions::FullDebugInfo);
   Inv->getTargetOpts().Triple = llvm::sys::getDefaultTargetTriple();
 
   Ins->setInvocation(std::move(Inv));
Index: clang/lib/Frontend/Rewrite/FrontendActions.cpp
===
--- clang/lib/Frontend/Rewrite/FrontendActions.cpp
+++ clang/lib/Frontend/Rewrite/FrontendActions.cpp
@@ -165,10 +165,11 @@
   if (std::unique_ptr OS =
   CI.createDefaultOutputFile(false, InFile, "cpp")) {
 if (CI.getLangOpts().ObjCRuntime.isNonFragile())
-  return CreateModernObjCRewriter(
-  std::string(InFile), std::move(OS), CI.getDiagnostics(),
-  CI.getLangOpts(), CI.getDiagnosticOpts().NoRewriteMacros,
-  (CI.getCodeGenOpts().getDebugInfo() != codegenoptions::NoDebugInfo));
+  return CreateModernObjCRewriter(std::string(InFile), std::move(OS),
+  CI.getDiagnostics(), CI.getLangOpts(),
+  CI.getDiagnosticOpts().NoRewriteMacros,
+  (CI.getCodeGenOpts().getDebugInfo() !=
+   llvm::codegenoptions::NoDebugInfo));
 return CreateObjCRewriter(std::string(InFile), std::move(OS),
   CI.getDiagnostics(), CI.getLangOpts(),
   CI.getDiagnosticOpts().NoRewriteMacros);
Index: clang/lib/Frontend/CompilerInvocation.cpp
===
--- clang/lib/Frontend/CompilerInvocation.cpp
+++ clang/lib/Frontend/CompilerInvocation.cpp
@@ -12,7 +12,6 @@
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/CommentOptions.h"
-#include "clang/Basic/DebugInfoOptions.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/DiagnosticDriver.h"
 #include "clang/Basic/DiagnosticOptions.h"
@@ -60,6 +59,7 @@
 #include "llvm/ADT/Triple.h"
 #include "llvm/ADT/Twine.h"
 #include "llvm/Config/llvm-config.h"
+#include "llvm/Frontend/Debug/Options.h"
 #include "llvm/IR/DebugInfoMetadata.h"
 #include "llvm/Linker/Linker.h"
 #include "llvm/MC/MCTargetOptions.h"
@@ -1366,28 +1366,28 @@
 
   std::optional DebugInfoVal;
   switch (Opts.DebugInfo) {
-  case codegenoptions::DebugLineTablesOnly:
+  case llvm::codegenoptions::DebugLineTablesOnly:
 DebugInfoVal = "line-tables-only";
 break;
-  case codegenoptions::Deb

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[Kiran Chandramohan via Phabricator via cfe-commits]

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

LGTM. Thanks for making the changes and for your patience. Please wait a couple 
of days to give other reviewers a chance to have a look before submitting.

Could you update the Summary of this patch to specify what is in this patch and 
what is left out? Also, might be useful to specify any special modelling, like 
for the map clause.

Could you specify the co-authors in the following way?
Co-authored-by: abidmalikwaterloo 
Co-authored-by: raghavendra 




Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:790
+//===-===//
+// 2.12.2 target data Construct
+//===-===//

Is this number from the OpenMP 5.0 standard? I think 5.0 does not have the 
`present` map-type modifier. The printer includes this. I think we can either 
remove things that are not there in 5.0 or add comments when items from newer 
versions are included or alternatively change the number to the latest version 
and call out everything that is not implemented.



Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:818-820
+The $map_operands specifies operands in map clause along with it's type 
and modifiers.
+
+The $map_operand_segments specifies the segment sizes for $map_operands.

Update these two to their current meanings. (Also replace 
`map_operand_segments` with `map_types`.

Same comment for the other two operations.



Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:542
+// Parser, printer and verifier for Target Data
+//===--===//
+static ParseResult

Could you specify the EBNF (https://mlir.llvm.org/docs/LangRef/#notation) for 
the expected structure of the map clause?



Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:595
+
+if (mapTypeOp.isa())
+  mapTypeBits = mapTypeOp.cast().getInt();

Nit: Should this be an assert?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915

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

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added inline comments.



Comment at: mlir/lib/Dialect/OpenMP/CMakeLists.txt:15
   MLIRLLVMDialect
+  MLIRArithDialect
   )

TIFitis wrote:
> kiranchandramohan wrote:
> > TIFitis wrote:
> > > kiranchandramohan wrote:
> > > > Why is this needed here?
> > > The Arith Dialect needs to be linked against as we're using it extract 
> > > the int value from arith.constant in the custom printer.
> > Can this be avoided by modelling constants as attributes?
> The issue with attributes is AFAIK `Variadic` is not supported, 
> and as previously discussed we need it be `Variadic` to support multiple map 
> clauses.
> 
> If I am wrong and there is indeed a way to have `Variadic` then 
> this can be avoided.
Can we use an ArrayAttr 
(https://mlir.llvm.org/docs/Dialects/Builtin/#arrayattr) or something derived 
from it?
https://github.com/llvm/llvm-project/blob/6ee5a1a090f3f4b6ae7ec9915900023277daf8d7/mlir/include/mlir/IR/OpBase.td#L1467


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915

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

[PATCH] D105584: [MLIR][OpenMP] Distribute Construct Operation

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added inline comments.



Comment at: mlir/test/Dialect/OpenMP/ops.mlir:124
 
+// CHECK-LABEL: omp_DistributeOp
+func.func @omp_DistributeOp(%lb : index, %ub : index, %step : index, %data_var 
: memref, %chunk_var : i32) -> () {

abidmalikwaterloo wrote:
> kiranchandramohan wrote:
> > kiranchandramohan wrote:
> > > Add a pretty-print test as well.
> > Nit: please add a pretty-print test.
> Just need clarification. Do you mean something similar to the following:
> 
> ```
> // CHECK-LABEL: omp_wsloop_pretty
> func.func @omp_wsloop_pretty(%lb : index, %ub : index, %step : index, 
> %data_var : memref, %linear_var : i32, %chunk_var : i32, %chunk_var2 : 
> i16) -> () {
> 
>   // CHECK: omp.wsloop ordered(2)
>   // CHECK-SAME: for (%{{.*}}) : index = (%{{.*}}) to (%{{.*}}) step (%{{.*}})
>   omp.wsloop ordered(2)
>   for (%iv) : index = (%lb) to (%ub) step (%step) {
> omp.yield
>   }
> 
>   // CHECK: omp.wsloop linear(%{{.*}} = %{{.*}} : memref) 
> schedule(static)
>   // CHECK-SAME: for (%{{.*}}) : index = (%{{.*}}) to (%{{.*}}) step (%{{.*}})
>   omp.wsloop schedule(static) linear(%data_var = %linear_var : memref)
>   for (%iv) : index = (%lb) to (%ub) step (%step) {
> omp.yield
>   }
> 
>   // CHECK: omp.wsloop linear(%{{.*}} = %{{.*}} : memref) 
> schedule(static = %{{.*}} : i32) ordered(2)
>   // CHECK-SAME: for (%{{.*}}) : index = (%{{.*}}) to (%{{.*}}) step (%{{.*}})
>   omp.wsloop ordered(2) linear(%data_var = %linear_var : memref) 
> schedule(static = %chunk_var : i32)
>   for (%iv) : index = (%lb) to (%ub) step (%step) {
> omp.yield
>   }
> 
>   // CHECK: omp.wsloop linear(%{{.*}} = %{{.*}} : memref) 
> schedule(dynamic = %{{.*}} : i32, nonmonotonic) ordered(2)
>   // CHECK-SAME: for (%{{.*}}) : index = (%{{.*}}) to (%{{.*}}) step (%{{.*}})
>   omp.wsloop ordered(2) linear(%data_var = %linear_var : memref) 
> schedule(dynamic = %chunk_var : i32, nonmonotonic)
>   for (%iv) : index = (%lb) to (%ub) step (%step)  {
> omp.yield
>   }
> 
>   // CHECK: omp.wsloop linear(%{{.*}} = %{{.*}} : memref) 
> schedule(dynamic = %{{.*}} : i16, monotonic) ordered(2)
>   // CHECK-SAME: for (%{{.*}}) : index = (%{{.*}}) to (%{{.*}}) step (%{{.*}})
>   omp.wsloop ordered(2) linear(%data_var = %linear_var : memref) 
> schedule(dynamic = %chunk_var2 : i16, monotonic)
>   for (%iv) : index = (%lb) to (%ub) step (%step) {
> omp.yield
>   }
> 
>   // CHECK: omp.wsloop for (%{{.*}}) : index = (%{{.*}}) to (%{{.*}}) step 
> (%{{.*}})
>   omp.wsloop for (%iv) : index = (%lb) to (%ub) step (%step) {
> omp.yield
>   }
> 
>   // CHECK: omp.wsloop for (%{{.*}}) : index = (%{{.*}}) to (%{{.*}}) 
> inclusive step (%{{.*}})
>   omp.wsloop for (%iv) : index = (%lb) to (%ub) inclusive step (%step) {
> omp.yield
>   }
> 
>   // CHECK: omp.wsloop nowait
>   // CHECK-SAME: for (%{{.*}}) : index = (%{{.*}}) to (%{{.*}}) step (%{{.*}})
>   omp.wsloop nowait
>   for (%iv) : index = (%lb) to (%ub) step (%step) {
> omp.yield
>   }
> 
>   // CHECK: omp.wsloop nowait order(concurrent)
>   // CHECK-SAME: for (%{{.*}}) : index = (%{{.*}}) to (%{{.*}}) step (%{{.*}})
>   omp.wsloop order(concurrent) nowait
>   for (%iv) : index = (%lb) to (%ub) step (%step) {
> omp.yield
>   }
> 
>   return
> }
> ```
Yes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105584

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

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan requested changes to this revision.
kiranchandramohan added a comment.
This revision now requires changes to proceed.

Can you add the other authors as Co-authors in the patch Summary?

In D131915#4001620 , @TIFitis wrote:

>> Can you add this as a test? AFAIS, the tests attached to this patch do not 
>> seem to be exercising the `VariadicofVariadic` requirement. An explanation 
>> with an example would be great.
>
> `VariadicOfVariadic` gives us a `SmallVector` where `ValueRange` 
> is essentially a `SmallVector`. As it is possible to have multiple map 
> clauses for a single target construct, this allows us to represent each map 
> clause as a row in `$map_operands`.
>
> I've updated the ops.mlir test to use two map clauses which better shows this 
> use case.
>
>> 1. Fortran Source + OpenMP example that needs a VariadicOfVariadic
>> 2. MLIR OpenMP representation
>
> Fortran Source:
>
>   subroutine omp_target_enter
>  integer :: a(1024)
>  integer :: b(1024)
>  !$omp target enter data map(to: a,) map(always, alloc: b)
>   end subroutine omp_target_enter
>
> MLIR OpenMP representation:
>
>   func.func @_QPomp_target_enter() {
> %0 = fir.alloca !fir.array<1024xi32> {bindc_name = "a", uniq_name = 
> "_QFomp_target_enterEa"}
> %1 = fir.alloca !fir.array<1024xi32> {bindc_name = "b", uniq_name = 
> "_QFomp_target_enterEb"}
> %c1_i64 = arith.constant 1 : i64
> %c4_i64 = arith.constant 4 : i64
> omp.target_enter_data   map((%c1_i64 -> none , to : %0 : 
> !fir.ref>), (%c4_i64 -> always , alloc : %1 : 
> !fir.ref>))
> return
>   }

Thanks, that helps.

> I plan on adding these as tests along with Fortran lowering support in 
> upcoming patches.
>
>> I am assuming we would need a verifier as well for the map clause.
>
> AFAIK the map clause rules are op specific. I plan on adding verifiers for 
> the ops soon in a separate patch.
>
>> Can you also restrict this patch to one of the constructs say `target data` 
>> for this patch? Once we decide on that then the other two can be easily 
>> added in a separate patch.
>
> Since I didn't make any changes to the ops I've left them in. If the patch 
> requires further changes, I'll leave them out.

The concern is with the introduction of VariadicOfVariadic with `AnyType`. I 
think this is new in the OpenMP Dialect and it pretty much allows anything. So, 
I was thinking whether it would be a good idea to write a verifier for the map 
clause, if not for the entire construct.




Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:826-827
+  Optional:$device,
+  Variadic:$use_device_ptr,
+  Variadic:$use_device_addr,
+  VariadicOfVariadic:$map_operands,

This is OK for now, but we might have to switch to `OpenMPPointerType` later.



Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:829
+  VariadicOfVariadic:$map_operands,
+  DenseI32ArrayAttr:$map_operand_segments);
+

Is `map_operand_segments` currently unused?



Comment at: mlir/lib/Dialect/OpenMP/CMakeLists.txt:15
   MLIRLLVMDialect
+  MLIRArithDialect
   )

TIFitis wrote:
> kiranchandramohan wrote:
> > Why is this needed here?
> The Arith Dialect needs to be linked against as we're using it extract the 
> int value from arith.constant in the custom printer.
Can this be avoided by modelling constants as attributes?



Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:577-583
+if (auto constOp =
+mlir::dyn_cast(a.front().getDefiningOp()))
+  mapTypeBits = constOp.getValue()
+.cast()
+.getValue()
+.getSExtValue();
+else if (auto constOp = mlir::dyn_cast(

Generally, constant values are modelled as attributes in MLIR representation. 
Can we switch to that representation? This will also avoid the need for this 
`if-else` and dependence on the `arith` dialect.



Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:595
+
+std::stringstream typeMod, type;
+if (always)

I think llvm prefers using `llvm::raw_string_ostream`. I have seen only two 
uses of std::stringstream in MLIR code, possibly accidentally introduced.



Comment at: mlir/test/Dialect/OpenMP/ops.mlir:389
+// CHECK: omp.target_data
+"omp.target_data"(%if_cond, %device, %data1, %data2) ({
+   

TIFitis wrote:
> kiranchandramohan wrote:
> > TIFitis wrote:
> > > clementval wrote:
> > > > Can you switch to the custom format form?
> > > Hi, I've updated the test file, Is this what you wanted?
> > Both the input and the CHECK lines in the custom format.
> I've changed both to custom format. Added a second map clause argument to 
> show support.
Can you increase the coverage s

[PATCH] D131915: [MLIR][OpenMP] Added target data, exit data, and enter data operation definition for MLIR.

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan requested changes to this revision.
kiranchandramohan added a comment.
This revision now requires changes to proceed.

Thanks for the changes and your work here.

In D131915#3994160 , @TIFitis wrote:

> Added custom printer & parser for map clause. Updated ops.mlir test to 
> address reviewer comments.
>
> Custom Printer example:
>
> OMP: !$omp target exit data map(from: a,b) map(release: c) map(delete: d) 
> map(always, from: e)
> CustomPrinter: omp.target_exit_data   map((%c2_i64 -> none , from : %0 : 
> !fir.ref>), (%c2_i64 -> none , from : %1 : 
> !fir.ref>), (%c0_i64 -> none , release : %2 : 
> !fir.ref>), (%c8_i64 -> none , delete : %3 : 
> !fir.ref>), (%c6_i64 -> always , from : %4 : 
> !fir.ref>))

Can you add this as a test? AFAIS, the tests attached to this patch do not seem 
to be exercising the `VariadicofVariadic` requirement. An explanation with an 
example would be great.

1. Fortran Source + OpenMP example that needs a VariadicOfVariadic
2. MLIR OpenMP representation

I am assuming we would need a verifier as well for the map clause.

Can you also restrict this patch to one of the constructs say `target data` for 
this patch? Once we decide on that then the other two can be easily added in a 
separate patch.




Comment at: mlir/lib/Dialect/OpenMP/CMakeLists.txt:15
   MLIRLLVMDialect
+  MLIRArithDialect
   )

Why is this needed here?



Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:572
+  for (const auto &a : map_operands) {
+auto mapTypeBits = 0x00;
+if (auto constOp =

Nit: can you specify the type here?



Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:584
+std::stringstream typeMod, type;
+if (mapTypeBits & 0x04)
+  typeMod << "always ";

There is a `bitn` function in `printSynchronizationHint` and 
`verifySynchronizationHint`, can that be used here?



Comment at: mlir/test/Dialect/OpenMP/ops.mlir:389
+// CHECK: omp.target_data
+"omp.target_data"(%if_cond, %device, %data1, %data2) ({
+   

TIFitis wrote:
> clementval wrote:
> > Can you switch to the custom format form?
> Hi, I've updated the test file, Is this what you wanted?
Both the input and the CHECK lines in the custom format.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131915

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

[PATCH] D105584: [MLIR][OpenMP] Distribute Construct Operation

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan requested changes to this revision.
kiranchandramohan added a comment.
This revision now requires changes to proceed.

@abidmalikwaterloo It seems you missed a few of the previous comments. Please 
fix these so that we can approve.




Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:449
  ParentOneOf<["WsLoopOp", "ReductionDeclareOp", 
- "AtomicUpdateOp", "SimdLoopOp"]>]> {
+ "AtomicUpdateOp", "SimdLoopOp","DistributeOp"]>]> {
   let summary = "loop yield and termination operation";

clementval wrote:
> abidmalikwaterloo wrote:
> > clementval wrote:
> > > 
> > What's the problem here? It seems fine to me.
> syntax doesn't match the existing code. Change is highlighted.
Nit: Please fix this (adding a space before `"DistributeOp"`).



Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:211
+//===-===//
+// Verifier for Dristribute Op
+//===-===//

kiranchandramohan wrote:
> 
Nit: Please make this change.



Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:220
+return emitOpError() << "empty upperbound for distribute loop operation";
+  
+  return success();

kiranchandramohan wrote:
> Nit: Check that their sizes are equal as well. And also if step is present 
> then its size matches lowerbound/upperbound.
Nit: Please make this change as well.



Comment at: mlir/test/Dialect/OpenMP/ops.mlir:124
 
+// CHECK-LABEL: omp_DistributeOp
+func.func @omp_DistributeOp(%lb : index, %ub : index, %step : index, %data_var 
: memref, %chunk_var : i32) -> () {

kiranchandramohan wrote:
> Add a pretty-print test as well.
Nit: please add a pretty-print test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105584

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

[PATCH] D105584: [MLIR][OpenMP] Distribute Construct Operation

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

In D105584#3954144 , 
@abidmalikwaterloo wrote:

> In D105584#3917238 , 
> @kiranchandramohan wrote:
>
>> Patch probably needs a rebase. A few more minor things to fix. Looks mostly 
>> ready.
>
> With which branch should I rebase?  I have taken care rest of the comments. 
> Thanks!

Rebase to the main branch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105584

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

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

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

Thanks @mnadeem for this patch. A few minor comments first. Try to replace auto 
in all places except where the type is on the RHS.

We might need `-fc1` tests as well.




Comment at: clang/lib/Driver/ToolChains/Flang.cpp:85
+ ArgStringList &CmdArgs) const {
+  // return;
+  const auto &TC = getToolChain();

Nit: leftover code?



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:86
+  // return;
+  const auto &TC = getToolChain();
+  const llvm::Triple &Triple = TC.getEffectiveTriple();

Nit: replace auto.



Comment at: clang/lib/Driver/ToolChains/Flang.cpp:99
+  default:
+// Untested for other targets but should work generally.
+break;

I get a segfault in ` Fortran::frontend::CodeGenAction::setUpTargetMachine()` 
currently when using `./bin/flang-new --target=x86_64-linux-gnu  file.f90`



Comment at: flang/lib/Frontend/CompilerInvocation.cpp:179
+
+  for (const auto *currentArg :
+   args.filtered(clang::driver::options::OPT_target_feature))

Nit: Can you remove the auto here?



Comment at: flang/lib/Frontend/FrontendActions.cpp:594
 
-  const std::string &theTriple = ci.getInvocation().getTargetOpts().triple;
+  const auto &targetOpts = ci.getInvocation().getTargetOpts();
+  const std::string &theTriple = targetOpts.triple;

Nit: Can you remove the auto here?


Repository:
  rG LLVM Github Monorepo

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

[PATCH] D105584: [MLIR][OpenMP] Distribute Construct Operation

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan requested changes to this revision.
kiranchandramohan added a comment.
This revision now requires changes to proceed.

Patch probably needs a rebase. A few more minor things to fix. Looks mostly 
ready.




Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:490
+
+The optional `dist_schedule_var` attribute specifies the  schedule for this
+loop, determining how the loop is distributed across the parallel threads.

Do you mean `static_dist_schedule` here?



Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:495-496
+
+The optional `collapse` attribute specifies the number of loops which
+are collapsed to form the distribute loop.
+  }];

We can remove `collapse` for now.



Comment at: mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td:513
+   ( $schedule_chunk^ `:` type($schedule_chunk))? `)`
+  |`collapse` `(` $collapse_val `)`
+  | `allocate` `(`

In https://reviews.llvm.org/D128338 we removed collapse from all constructs. 
Currently collapse is modelled by having multiple entries in the lowerBound, 
upperBound and step.



Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:211
+//===-===//
+// Verifier for Dristribute Op
+//===-===//





Comment at: mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp:220
+return emitOpError() << "empty upperbound for distribute loop operation";
+  
+  return success();

Nit: Check that their sizes are equal as well. And also if step is present then 
its size matches lowerbound/upperbound.



Comment at: mlir/test/Dialect/OpenMP/ops.mlir:124
 
+// CHECK-LABEL: omp_DistributeOp
+func.func @omp_DistributeOp(%lb : index, %ub : index, %step : index, %data_var 
: memref, %chunk_var : i32) -> () {

Add a pretty-print test as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105584

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

[PATCH] D137329: [flang] Add -f[no-]associative-math and -mreassociate

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

In D137329#3909943 , @awarzynski 
wrote:

> In D137329#3909082 , @clementval 
> wrote:
>
>> Wouldn't it be good to have a RFC for all these options and what they will 
>> do in Flang instead of just adding them all? Or did I miss the RFC?
>
> +1

Fast Math attributes supported in LLVM IR are documented in 
https://llvm.org/docs/LangRef.html#fast-math-flags. This set of patches 
(details below) provides a way to set or unset each of these attributes.

While making policy decisions about these flags, like for ffp-contract, we have 
created an RFC 
(https://discourse.llvm.org/t/rfc-ffp-contract-default-value/66301).  When 
adding umbrella flags like 
-ffast-math/-Ofast/-ffp-model/-funsafe-math-optimizations, we will submit RFCs.

Patch : Flag Name : LLVM IR Attribute
https://reviews.llvm.org/D137325 : f[no-]honor-nans: nnan
https://reviews.llvm.org/D137072 : f[no-]honor-infinities  : ninf
https://reviews.llvm.org/D137328 : f[no-]signed-zeros  : nsz
https://reviews.llvm.org/D137330 : f[no-]reciprocal-math : arcp
https://reviews.llvm.org/D136080 : ffp-contract option : contract
https://reviews.llvm.org/D137326 : f[no-]approx-func: afn
https://reviews.llvm.org/D137329 : f[no-]associative-math: reassoc


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137329

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

[PATCH] D136080: [flang] Add -ffp-contract option processing

[Kiran Chandramohan via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rGa784de783af5: [flang] Add -ffp-contract option processing 
(authored by tblah, committed by kiranchandramohan).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136080

Files:
  clang/include/clang/Basic/DiagnosticDriverKinds.td
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Flang.cpp
  flang/include/flang/Frontend/CompilerInvocation.h
  flang/include/flang/Frontend/LangOptions.def
  flang/include/flang/Frontend/LangOptions.h
  flang/lib/Frontend/CMakeLists.txt
  flang/lib/Frontend/CompilerInvocation.cpp
  flang/lib/Frontend/LangOptions.cpp
  flang/test/Driver/driver-help-hidden.f90
  flang/test/Driver/driver-help.f90
  flang/test/Driver/flang_f_opts.f90
  flang/test/Driver/flang_fp_opts.f90
  flang/test/Driver/frontend-forwarding.f90

Index: flang/test/Driver/frontend-forwarding.f90
===
--- flang/test/Driver/frontend-forwarding.f90
+++ flang/test/Driver/frontend-forwarding.f90
@@ -8,6 +8,7 @@
 ! RUN: -fdefault-real-8 \
 ! RUN: -flarge-sizes \
 ! RUN: -fconvert=little-endian \
+! RUN: -ffp-contract=fast \
 ! RUN: -mllvm -print-before-all\
 ! RUN: -P \
 ! RUN:   | FileCheck %s
@@ -18,5 +19,6 @@
 ! CHECK: "-fdefault-integer-8"
 ! CHECK: "-fdefault-real-8"
 ! CHECK: "-flarge-sizes"
+! CHECK: "-ffp-contract=fast"
 ! CHECK: "-fconvert=little-endian"
 ! CHECK:  "-mllvm" "-print-before-all"
Index: flang/test/Driver/flang_fp_opts.f90
===
--- /dev/null
+++ flang/test/Driver/flang_fp_opts.f90
@@ -0,0 +1,4 @@
+! Test for handling of floating point options within the frontend driver
+
+! RUN: %flang_fc1 -ffp-contract=fast %s 2>&1 | FileCheck %s
+! CHECK: ffp-contract= is not currently implemented
Index: flang/test/Driver/flang_f_opts.f90
===
--- flang/test/Driver/flang_f_opts.f90
+++ flang/test/Driver/flang_f_opts.f90
@@ -1,8 +1,10 @@
 ! Test for warnings generated when parsing driver options. You can use this file for relatively small tests and to avoid creating
 ! new test files.
 
-! RUN: %flang -### -S -O4 %s 2>&1 | FileCheck %s
+! RUN: %flang -### -S -O4 -ffp-contract=on %s 2>&1 | FileCheck %s
 
+! CHECK: warning: the argument 'on' is not supported for option 'ffp-contract='. Mapping to 'ffp-contract=off'
 ! CHECK: warning: -O4 is equivalent to -O3
 ! CHECK-LABEL: "-fc1"
+! CHECK: -ffp-contract=off
 ! CHECK: -O3
Index: flang/test/Driver/driver-help.f90
===
--- flang/test/Driver/driver-help.f90
+++ flang/test/Driver/driver-help.f90
@@ -31,6 +31,7 @@
 ! HELP-NEXT: -ffixed-form   Process source files in fixed form
 ! HELP-NEXT: -ffixed-line-length=
 ! HELP-NEXT: Use  as character line width in fixed mode
+! HELP-NEXT: -ffp-contract= Form fused FP ops (e.g. FMAs)
 ! HELP-NEXT: -ffree-formProcess source files in free form
 ! HELP-NEXT: -fimplicit-noneNo implicit typing allowed unless overridden by IMPLICIT statements
 ! HELP-NEXT: -finput-charset= Specify the default character set for source files
@@ -105,6 +106,7 @@
 ! HELP-FC1-NEXT: -ffixed-form   Process source files in fixed form
 ! HELP-FC1-NEXT: -ffixed-line-length=
 ! HELP-FC1-NEXT: Use  as character line width in fixed mode
+! HELP-FC1-NEXT: -ffp-contract= Form fused FP ops (e.g. FMAs)
 ! HELP-FC1-NEXT: -ffree-formProcess source files in free form
 ! HELP-FC1-NEXT: -fget-definition   
 ! HELP-FC1-NEXT:Get the symbol definition from   
Index: flang/test/Driver/driver-help-hidden.f90
===
--- flang/test/Driver/driver-help-hidden.f90
+++ flang/test/Driver/driver-help-hidden.f90
@@ -31,6 +31,7 @@
 ! CHECK-NEXT: -ffixed-form   Process source files in fixed form
 ! CHECK-NEXT: -ffixed-line-length=
 ! CHECK-NEXT: Use  as character line width in fixed mode
+! CHECK-NEXT: -ffp-contract= Form fused FP ops (e.g. FMAs)
 ! CHECK-NEXT: -ffree-formProcess source files in free form
 ! CHECK-NEXT: -fimplicit-noneNo implicit typing allowed unless overridden by IMPLICIT statements
 ! CHECK-NEXT: -finput-charset= Specify the default character set for source files
Index: flang/lib/Frontend/LangOptions.cpp
===
--- /dev/null
+++ flang/lib/Frontend/LangOptions.cpp
@@ -0,0 +1,24 @@
+//===-- LangOptions.cpp ---===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: 

[PATCH] D131808: [clang,flang] Add help text for -fsyntax-only

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

In D131808#3722809 , @alexiprof wrote:

> Could someone land this patch please?
> Alexander Malkov 

Was landed with the email id contained in the patch (Author: Alexander Malkov 
). Hope that is OK. May be you can update to the 
gmail id if that is what you prefer in your phabricator account.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131808

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

[PATCH] D131808: [clang,flang] Add help text for -fsyntax-only

[Kiran Chandramohan via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGcd86a03246a2: [clang,flang] Add help text for -fsyntax-only 
(authored by alexiprof, committed by kiranchandramohan).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131808

Files:
  clang/docs/ClangCommandLineReference.rst
  clang/docs/CommandGuide/clang.rst
  clang/docs/UsersManual.rst
  clang/include/clang/Driver/Options.td
  flang/test/Driver/driver-help-hidden.f90
  flang/test/Driver/driver-help.f90

Index: flang/test/Driver/driver-help.f90
===
--- flang/test/Driver/driver-help.f90
+++ flang/test/Driver/driver-help.f90
@@ -42,6 +42,7 @@
 ! HELP-NEXT: -fno-integrated-as  Disable the integrated assembler
 ! HELP-NEXT: -fopenacc  Enable OpenACC
 ! HELP-NEXT: -fopenmp   Parse OpenMP pragmas and generate parallel code.
+! HELP-NEXT: -fsyntax-only  Run the preprocessor, parser and semantic analysis stages
 ! HELP-NEXT: -fxor-operator Enable .XOR. as a synonym of .NEQV.
 ! HELP-NEXT: -help  Display available options
 ! HELP-NEXT: -IAdd directory to the end of the list of include search paths
@@ -119,6 +120,7 @@
 ! HELP-FC1-NEXT: -fno-reformat  Dump the cooked character stream in -E mode
 ! HELP-FC1-NEXT: -fopenacc  Enable OpenACC
 ! HELP-FC1-NEXT: -fopenmp   Parse OpenMP pragmas and generate parallel code.
+! HELP-FC1-NEXT: -fsyntax-only  Run the preprocessor, parser and semantic analysis stages
 ! HELP-FC1-NEXT: -fxor-operator Enable .XOR. as a synonym of .NEQV.
 ! HELP-FC1-NEXT: -help  Display available options
 ! HELP-FC1-NEXT: -init-only Only execute frontend initialization
Index: flang/test/Driver/driver-help-hidden.f90
===
--- flang/test/Driver/driver-help-hidden.f90
+++ flang/test/Driver/driver-help-hidden.f90
@@ -44,6 +44,7 @@
 ! CHECK-NEXT: -fno-integrated-as Disable the integrated assembler
 ! CHECK-NEXT: -fopenacc  Enable OpenACC
 ! CHECK-NEXT: -fopenmp   Parse OpenMP pragmas and generate parallel code.
+! CHECK-NEXT: -fsyntax-only  Run the preprocessor, parser and semantic analysis stages
 ! CHECK-NEXT: -fxor-operator Enable .XOR. as a synonym of .NEQV.
 ! CHECK-NEXT: -help Display available options
 ! CHECK-NEXT: -IAdd directory to the end of the list of include search paths
Index: clang/include/clang/Driver/Options.td
===
--- clang/include/clang/Driver/Options.td
+++ clang/include/clang/Driver/Options.td
@@ -2800,7 +2800,8 @@
 def fdriver_only : Flag<["-"], "fdriver-only">, Flags<[NoXarchOption, CoreOption]>,
   Group, HelpText<"Only run the driver.">;
 def fsyntax_only : Flag<["-"], "fsyntax-only">,
-  Flags<[NoXarchOption,CoreOption,CC1Option,FC1Option]>, Group;
+  Flags<[NoXarchOption,CoreOption,CC1Option,FC1Option,FlangOption]>, Group,
+  HelpText<"Run the preprocessor, parser and semantic analysis stages">;
 def ftabstop_EQ : Joined<["-"], "ftabstop=">, Group;
 def ftemplate_depth_EQ : Joined<["-"], "ftemplate-depth=">, Group;
 def ftemplate_depth_ : Joined<["-"], "ftemplate-depth-">, Group;
@@ -6618,7 +6619,7 @@
 def _SLASH_Zp_flag : CLFlag<"Zp">,
   HelpText<"Set default maximum struct packing alignment to 1">,
   Alias, AliasArgs<["1"]>;
-def _SLASH_Zs : CLFlag<"Zs">, HelpText<"Syntax-check only">,
+def _SLASH_Zs : CLFlag<"Zs">, HelpText<"Run the preprocessor, parser and semantic analysis stages">,
   Alias;
 def _SLASH_openmp_ : CLFlag<"openmp-">,
   HelpText<"Disable OpenMP support">, Alias;
Index: clang/docs/UsersManual.rst
===
--- clang/docs/UsersManual.rst
+++ clang/docs/UsersManual.rst
@@ -3999,7 +3999,7 @@
   /Zl Don't mention any default libraries in the object file
   /Zp Set the default maximum struct packing alignment to 1
   /Zp  Specify the default maximum struct packing alignment
-  /Zs Syntax-check only
+  /Zs Run the preprocessor, parser and semantic analysis stages
 
 OPTIONS:
   -###Print (but do not run) the commands to run for this compilation
@@ -4130,6 +4130,7 @@
   behavior. See user manual for available checks
   -fsplit-lto-unitEnables splitting of the LTO unit.
   -fstandalone-debug  Emit full debug info for all types used by the program
+  -fsyntax-only   Run the preprocessor, parser and semantic analysis stages
   -fwhole-program-vtables Enables whole-program vtable optimization. Requires -flto
   -gcodeview-ghash

[PATCH] D131808: [clang,flang] Add help text for -fsyntax-only

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

I can land this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131808

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

[PATCH] D129149: [OMPIRBuilder] Add support for simdlen clause

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

In D129149#3632861 , @psoni2628 wrote:

> In D129149#3632813 , 
> @kiranchandramohan wrote:
>
>> Nit: Also add to the summary that this patch uses the simdlen support in 
>> OpenMPIRBuilder when it is enabled in Clang.
>
> I just wanted to clarify that below is what you mean?
>
>   This patch adds OMPIRBuilder support for the simdlen clause for the
>   simd directive. It uses the simdlen support in OpenMPIRBuilder when
>   it is enabled in clang.

Yes. Thanks.

Maybe you can also add that simdlen is lowered by the OpenMPIRBuilder by 
generating the loop.vectorize.width metadata.


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

https://reviews.llvm.org/D129149

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

[PATCH] D129149: [OMPIRBuilder] Add support for simdlen clause

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

Nit: Also add to the summary that this patch uses the simdlen support in 
OpenMPIRBuilder when it is enabled in Clang.




Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:2600
+  continue;
+else
+  return false;

Nit: Else after return/continue is discouraged.
https://llvm.org/docs/CodingStandards.html#don-t-use-else-after-a-return


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

https://reviews.llvm.org/D129149

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

[PATCH] D110114: [OMPIRBuilder] Generate aggregate argument for parallel region outlined functions

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added subscribers: Meinersbur, kiranchandramohan.
kiranchandramohan added a comment.
Herald added a project: All.

If I understand this correctly, filling the aggregate struct is only happening 
in the parallel case but not for the serialized parallel version. See example 
below, the call to `@sb_..omp_par` from kmpc_fork_call in block `omp_parallel` 
has the aggregate filled in while the call to `@sb_..omp_par` in the serialized 
parallel region in block 6 does not have this. I assume that the extractor 
creates these and it will only do it at the place that it is called. Would 
copying over the instructions that fill the aggregate to the serialized 
parallel region be a reasonable solution?

CC: @Meinersbur

  define void @sb_(ptr %0, ptr %1) !dbg !3 {
%structArg = alloca { ptr }, align 8
%tid.addr = alloca i32, align 4, !dbg !7
%zero.addr = alloca i32, align 4, !dbg !7
store i32 0, ptr %tid.addr, align 4, !dbg !7
store i32 0, ptr %zero.addr, align 4, !dbg !7
%3 = load i32, ptr %0, align 4, !dbg !7
%4 = icmp ne i32 %3, 0, !dbg !7 
br label %entry, !dbg !7
  
  entry:; preds = %2
%omp_global_thread_num = call i32 @__kmpc_global_thread_num(ptr @1), !dbg !7
br i1 %4, label %5, label %6
  
  5:; preds = %entry
br label %omp_parallel
  
  omp_parallel: ; preds = %5
%gep_ = getelementptr { ptr }, ptr %structArg, i32 0, i32 0
store ptr %1, ptr %gep_, align 8
call void (ptr, i32, ptr, ...) @__kmpc_fork_call(ptr @1, i32 1, ptr 
@sb_..omp_par, ptr %structArg), !dbg !9
br label %omp.par.outlined.exit
  
  omp.par.outlined.exit:; preds = %omp_parallel
br label %omp.par.exit.split
  
  omp.par.exit.split:   ; preds = 
%omp.par.outlined.exit
br label %7
  
  6:; preds = %entry
call void @__kmpc_serialized_parallel(ptr @1, i32 %omp_global_thread_num)
call void @sb_..omp_par(ptr %tid.addr, ptr %zero.addr, ptr %structArg), 
!dbg !9
call void @__kmpc_end_serialized_parallel(ptr @1, i32 
%omp_global_thread_num)
br label %7
  
  7:; preds = %6, 
%omp.par.exit.split
ret void, !dbg !10
  }


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D110114

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

[PATCH] D128333: [clang][flang] Disable defaulting to `-fpie` for LLVM Flang

[Kiran Chandramohan via Phabricator via cfe-commits]

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

LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128333

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

[PATCH] D128333: [clang][flang] Disable defaulting to `-fpie` for LLVM Flang

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

Is the longer-term plan to support this in Flang as well? 
Does this affect building object files too or is it just the executable? How 
would this affect mixed C++/Fortran programs if the Clang and Flang settings 
are different?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128333

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

[PATCH] D125788: [flang][driver] Rename `flang-new` as `flang`

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

In D125788#3535199 , @sscalpone wrote:

> My proposal is:
>
> If the compiler compiles it, it ought to run.
> If the compiler can't compile it, it ought to clearly say why.
>
> 1. All tests of legal Fortran that compile & link must also execute correctly 
> (which excludes tests that expect to catch a problem at runtime)
> 2. For all tests with unsupported features, the compiler must issues an error 
> message and the message references the source-location of the unsupported 
> feature
>
> My preference is to use the NAG test suite.   It is not freely available.

I strongly suspect that you might be able to win over some of the folks by 
sharing the status/list of failures/list of issues to be fixed for getting the 
NAG test suite to pass. As I have mentioned before, at least the companies 
involved have access to the NAG testsuite. The community can probably help you 
achieve the outlined goals faster. It can also help focus our efforts.  Some of 
our engineers have now worked with f18/flang for some time. While some issues 
might require a fix by the experts, many can be fixed by others.

A separate issue we have is that it is not clear which branch is it that is 
being used for testing with NAG. Since many fixes have landed in 
llvm-project/flang, I assume that fir-dev is now out of date. While most of 
upstreaming is complete there is still a bit of things left to upstream, should 
not take much time though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125788

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

[PATCH] D126291: [flang][Driver] Update link job on windows

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

@rovka in case you missed this, the test is failing in windows.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126291

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

[PATCH] D125788: [flang][driver] Rename `flang-new` as `flang`

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

In D125788#3523408 , @shraiysh wrote:

>> With this change the flang driver will not generate an executable unless it 
>> is built with an option.
>
> Okay, thank you for the clarification. Is this a cmake option and? Are we 
> documenting this somewhere except this patch summary, like on the flang 
> documentation or something? Apologies if this a repetition to what you meant 
> by clarifying that it is available via a flag.

I think it is a driver flag `-flang-experimental-exec`. I was only requesting 
the add this information to the patch summary. If you feel it should be present 
somewhere else then please add a separate comment.

Some users might have been blindly running `flang` before without being aware 
that the script was calling an external compiler to generated code/executable. 
This change introduces the lowering flow and the users might see different 
results,
-> TODO messages for 2003 and above features.
-> Unexpected ICEs or miscomparisons.
-> Not producing an executable.

In D125788#3523443 , @rouson wrote:

> I think this is an exciting step.  I hope it gets approved.  Although it's 
> technically true that this could appear to be a regression for current flang 
> script users, the ultimate compilation currently happens by invoking an 
> external compiler so most current flang script users can eliminate the 
> regression by simply calling the external compiler.  The exception would be 
> if the current flang script user specifically wants flang's parsing 
> capabilities for checking code correctness (do we know if anyone is using the 
> flang script that way?) or for testing the parser (which I presume can still 
> happen by other means).  It might be nice to simply rename the current script 
> so those who want it can simply change the name they use to invoke it.

Yes, the user can use `flang-to-external-fc` if they want the old flow. But 
this change might be a surprise for the user hence the request to add the 
relevant information to the summary.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125788

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

[PATCH] D125788: [flang][driver] Rename `flang-new` as `flang`

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

In D125788#3523259 , @shraiysh wrote:

>> clarify that execution is still restricted to developers via a flag.
>
> Just confirming what this means: After this patch, would `flang sample.f90` 
> generate an executable? If not, how to generate an executable using 
> `flang-new`? If yes, does this mean that if I just replace classic-flang with 
> new llvm `flang` (along with library and include paths), I should be able to 
> find and report bugs (expected) or run the application (best case)?

Sorry, the point I was trying to say here is that till now the `flang` script 
will parse, unparse and call an external compiler to do the codegen/generate 
executable. With this change the `flang` driver will not generate an executable 
unless it is built with an option. This default behaviour might be seen as a 
regression from the point of view of a `flang` script user.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125788

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

[PATCH] D125788: [flang][driver] Rename `flang-new` as `flang`

[Kiran Chandramohan via Phabricator via cfe-commits]

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

LGTM. In the call on Monday, it was mentioned that the original users of the 
flang script have moved on to using a custom driver.

It will be a good idea to,
-> wait for the author of the original script and the throwaway driver, 
@sscalpone.
-> clarify that execution is still restricted to developers via a flag. We hope 
to open it to users later in the year.

My personal preference for the renaming is `flang-to-external-fc` since it is 
not specific to gfortran.




Comment at: flang/tools/flang-driver/CMakeLists.txt:51
 
-install(TARGETS flang-new DESTINATION "${CMAKE_INSTALL_BINDIR}")
+# For backwords compatibility
+add_flang_symlink(flang-new flang)

Nit: backwords -> backwards


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125788

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

[PATCH] D118409: [OpenMPIRBuilder] Remove ContinuationBB argument from Body callback.

[Kiran Chandramohan via Phabricator via cfe-commits]

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

LGTM. You may wait for a day in case there are comments from other reviewers.

I think the CI is complaining about some clang-format issue in  
clang/test/OpenMP/critical_codegen_attr.cpp


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118409

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

[PATCH] D118409: [OpenMPIRBuilder] Remove ContinuationBB argument from Body callback.

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

Thanks @Meinersbur for this patch. Apologies for the delay in reviewing many of 
your patches.

The patch looks mostly LGTM.  A few nits. The patch probably needs a rebase 
since there are no dependencies now and a few more construct lowering has 
landed in MLIR (single, simdloop).




Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:5571-5579
+  {
+OMPBuilderCBHelpers::InlinedRegionBodyRAII IRB(*this, AllocaIP,
+   *FiniBB);
+EmitStmt(CS->getCapturedStmt());
+  }
+
+  if (Builder.saveIP().isSet())

Why is it not possible to use `OMPBuilderCBHelpers::EmitOMPInlinedRegionBody` 
here?



Comment at: clang/lib/CodeGen/CodeGenFunction.h:1819
+/// Emit the body of an OMP region that will be outlined in
+/// OpenMPIRBuilder::finialize().
+/// \param CGF   The Codegen function this belongs to

Nit:finalize



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:26-58
+/// Move the instruction after an InsertPoint to the beginning of another
+/// BasicBlock.
+///
+/// The instructions after \p IP are moved to the beginning of \p New which 
must
+/// not have any PHINodes. If \p CreateBranch is true, a branch instruction to
+/// \p New will be added such that there is no semantic change. Otherwise, the
+/// \p IP insert block remains degenerate and it is up to the caller to insert 
a

I guess these are already in from your other patch 
(https://reviews.llvm.org/D114413).



Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:138
+  /// BasicBlock after CodeGenIP including CodeGenIP itself are invalidated. If
+  /// such InsertPoints need to be preserved, it can split the block itseff
+  /// before calling the callback.

Nit:itself



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1086
   auto FiniInfo = FinalizationStack.pop_back_val();
-  assert(FiniInfo.DK == OMPD_sections &&
- "Unexpected finalization stack state!");
-  Builder.SetInsertPoint(LoopAfterBB->getTerminator());
-  FiniInfo.FiniCB(Builder.saveIP());
-  Builder.SetInsertPoint(ExitBB);
+  assert(FiniInfo.DK == OMPD_sections && "Uxpected finalization stack state!");
+  if (FinalizeCallbackTy &CB = FiniInfo.FiniCB) {

Nit:Unexpected



Comment at: 
mlir/lib/Target/LLVMIR/Dialect/OpenMP/OpenMPToLLVMIRTranslation.cpp:103
+  llvm::BasicBlock *continuationBlock =
+  splitBB(builder, true, "omp.region.cont");
+  llvm::BasicBlock *sourceBlock = builder.GetInsertBlock();

Would this "omp.region.cont" be a mostly empty block in most cases? I guess it 
is not a problem since llvm will remove these blocks.

The IR generated for 
```
  omp.parallel {
omp.barrier
omp.terminator
  }
```
is the following. Notice the empty (only a branch) `omp.region.cont` block. 
Previously we had this only at the source side `omp.par.region`.

```
omp.par.entry:
  %tid.addr.local = alloca i32, align 4
  %0 = load i32, i32* %tid.addr, align 4
  store i32 %0, i32* %tid.addr.local, align 4
  %tid = load i32, i32* %tid.addr.local, align 4
  br label %omp.par.region

omp.par.region:   ; preds = %omp.par.entry
  br label %omp.par.region1

omp.par.region1:  ; preds = %omp.par.region
  %omp_global_thread_num2 = call i32 @__kmpc_global_thread_num(%struct.ident_t* 
@4)
  call void @__kmpc_barrier(%struct.ident_t* @3, i32 %omp_global_thread_num2)
  br label %omp.region.cont, !dbg !12

omp.region.cont:  ; preds = %omp.par.region1
  br label %omp.par.pre_finalize

omp.par.pre_finalize: ; preds = %omp.region.cont
  br label %omp.par.outlined.exit.exitStub
```



Comment at: openmp/runtime/test/ompt/cancel/cancel_parallel.c:1
-// RUN: %libomp-compile && env OMP_CANCELLATION=true %libomp-run | 
%sort-threads | FileCheck %s
+// RUN: %libomp-compile && env OMP_CANCELLATION=true %libomp-run \
+//  | %sort-threads | FileCheck %s

Nit: unrelated change?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118409

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

[PATCH] D119012: [flang][driver] Add support for the `-emit-llvm` option

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

The tests added are failing in the windows CI.




Comment at: flang/lib/Frontend/FrontendActions.cpp:421
+  // Set-up the MLIR pass manager
+  fir::setTargetTriple(*mlirModule_, "native");
+  auto &defKinds = ci.invocation().semanticsContext().defaultKinds();

awarzynski wrote:
> rovka wrote:
> > Nit: Should we assert that mlirModule exists?
> > Also, why doesn't it already have a triple and a kind mapping?
> > Nit: Should we assert that mlirModule exists?
> 
> We could do, yes. However, `mlirModule` is created in 
> `CodeGenAction::BeginSourceFileAction`. If that method fails, the driver 
> should stop immediately. So perhaps that would be a better for place for an 
> assert?
> 
> On a related note, `mlirModule` is obtained via [[ 
> https://github.com/llvm/llvm-project/blob/79b3fe80707b2eb9a38c1517a829fb58062fb687/flang/include/flang/Lower/Bridge.h#L67
>  | LoweringBridge::getModule ]]. But if the corresponding module is `nullptr` 
> than that getter should probably assert. See https://reviews.llvm.org/D119133.
> 
> >  Also, why doesn't it already have a triple and a kind mapping?
> Good catch, this is not needed yet. I didn't notice this when going over 
> `tco`.
 D118985 creates a bridge with an empty triple. The patch here was switching it 
to "native". Please cross-check what the expected behaviour.

```
  lower::LoweringBridge lb = Fortran::lower::LoweringBridge::create(*mlirCtx,
  defKinds, ci.invocation().semanticsContext().intrinsics(),
  ci.parsing().allCooked(), "", kindMap);
```



Comment at: flang/lib/Frontend/FrontendActions.cpp:440
+  // Translate to LLVM IR
+  auto optName = mlirModule->getName();
+  llvmCtx = std::make_unique();

Nit: Remove auto.



Comment at: flang/unittests/Frontend/FrontendActionTest.cpp:176
+  // stream, as opposed to an actual file (or a file descriptor).
+  llvm::SmallVector outputFileBuffer;
+  std::unique_ptr outputFileStream(

Nit: Is the size important?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119012

___
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`

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan accepted this revision.
kiranchandramohan added a comment.

LGTM.




Comment at: flang/lib/Frontend/FrontendActions.cpp:419
+
+  // ... otherwise, print to a file.
+  auto os{ci.CreateDefaultOutputFile(

awarzynski wrote:
> kiranchandramohan wrote:
> > Nit: Should we test the existence of such a file?
> We do :) Sort of!
> 
> To me, "existence" is a low level concept. Files are handled through e.g. 
> `llvm::raw_fd_ostream` (and occasionally other streams) and IMO all low level 
> details should be dealt with there (and indeed are). `flang-new` should only 
> verify that `os` is not a `nullptr`. If the file does not exist, `os` will be 
> a `nullptr` and that's checked further down. If the file does exist, then 
> everything is fine and we can move to the next step. 
OK, that was sounds fine. I was also meaning to ask whether we should have a 
test that a *.mlir file is generated in a test.


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] D118985: [flang][driver] Add support for `-emit-mlir`

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

Few Nits/questions.




Comment at: flang/lib/Frontend/FrontendActions.cpp:69
+
+  // Create a LoweringBridge
+  auto &defKinds = ci.invocation().semanticsContext().defaultKinds();

Nit: Can we remove the three autos below?



Comment at: flang/lib/Frontend/FrontendActions.cpp:419
+
+  // ... otherwise, print to a file.
+  auto os{ci.CreateDefaultOutputFile(

Nit: Should we test the existence of such a file?



Comment at: flang/lib/Frontend/FrontendActions.cpp:420
+  // ... otherwise, print to a file.
+  auto os{ci.CreateDefaultOutputFile(
+  /*Binary=*/true, /*InFile=*/GetCurrentFileOrBufferName(), "mlir")};

Nit: can we remove auto?



Comment at: flang/test/Driver/emit-mlir.f90:1
+! The the `-emit-fir` option
+

Nit: The the



Comment at: flang/test/Driver/syntax-only.f90:16
-! FSYNTAX_ONLY: IF statement is not allowed in IF statement
-! FSYNTAX_ONLY_NEXT: Semantic errors in {{.*}}syntax-only.f90
-

Nit: Do you have another test for `fsyntax-only`?



Comment at: flang/test/Driver/syntax-only.f90:23
+! CHECK-NOT: error
 ! NO_FSYNTAX_ONLY: error: code-generation is not available yet
 

Do we currently run the stages before codegen and then issue this error?


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] D111992: [MLIR][OpenMP] Added omp.atomic.read and omp.atomic.write

[Kiran Chandramohan via Phabricator via cfe-commits]

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

LGTM.

My preference is the following one. If you agree then please make the change, 
otherwise, you can keep it as is. Also, wait a couple of days to see whether 
others have comments.

  %8 = omp.atomic.read %addr hint(speculative, contended) memory_order(seq_cst) 
: memref -> i32


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111992

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

[PATCH] D111992: [MLIR][OpenMP] Added omp.atomic.read and omp.atomic.write

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

The patch looks OK. I just wanted to discuss the syntax also. Would any of the 
following be better?

  %8 = omp.atomic.read %addr : memref -> i32 hint(speculative, contended) 
memory_order(seq_cst) 

  %8 = omp.atomic.read %addr hint(speculative, contended) memory_order(seq_cst) 
: memref -> i32 

  %8 = omp.atomic.read %addr {hint(speculative, contended) 
memory_order(seq_cst)} : memref -> i32


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D111992

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

[PATCH] D107430: [OMPIRBuilder] Add ordered directive to OMPIRBuilder

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan accepted this revision.
kiranchandramohan added a comment.

LGTM.


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

https://reviews.llvm.org/D107430

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

[PATCH] D107430: [OMPIRBuilder] Add ordered directive to OMPIRBuilder

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

Thanks @peixin. I am going through the patch today and will accept or provide 
some comments.


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

https://reviews.llvm.org/D107430

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

[PATCH] D107764: [OpenMP][OpenMPIRBuilder] Implement loop unrolling.

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan accepted this revision.
kiranchandramohan added a comment.

LGTM. Thanks for the responses.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107764

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

[PATCH] D107764: [OpenMP][OpenMPIRBuilder] Implement loop unrolling.

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

I have a few minor questions.




Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2055
+/// Attach loop metadata \p Properties to the loop described by \p Loop. If the
+/// loop already has metadata, the loop properties are appended.
+static void addLoopMetadata(CanonicalLoopInfo *Loop,

Nit: In the body, it seems we are prepending.



Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:2168-2169
+  // actually unrolls the loop.
+  UP.Threshold *= 1.5;
+  UP.PartialThreshold *= 1.5;
+

Should this be settable for experimentation or additional control? If not can 
you provide an explanation for these values?



Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1666
 
+TEST_F(OpenMPIRBuilderTest, UnrollLoopFull) {
+  OpenMPIRBuilder OMPBuilder(*M);

Are we not calling ompbuilder finalize or verify because it is only adding 
metadata?



Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1735
+
 TEST_F(OpenMPIRBuilderTest, StaticWorkShareLoop) {
   using InsertPointTy = OpenMPIRBuilder::InsertPointTy;

Should we add tests for workshare loops with unroll?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107764

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

[PATCH] D107430: [OMPIRBuilder] Add ordered without depend and simd clause to OMPBuilder

[Kiran Chandramohan via Phabricator via cfe-commits]

kiranchandramohan added a comment.

Thanks @peixin for the patch. Could you remove the OpenMP 1.0 target from the 
description? That is a target for the Flang team and since this patch touches 
clang as well, it might be confusing. I believe this is modelling the ordered 
construct with a region. Could simd be another option to CreateOrdered or does 
it need more changes? Also, could you clarify how the standalone ordered 
construct (with depend) will be handled? Will it be an extension to 
CreateOrdered or will be a completely separate function?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D107430

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

[PATCH] D93373: [Flang][Openmp] Upgrade TASKGROUP construct to 5.0.

[Kiran Chandramohan via Phabricator via cfe-commits]

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

LGTM. Please update the commit message with information about changes in Clang 
tests as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93373

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