Page MenuHomePhabricator

kiranchandramohan (Kiran Chandramohan)
User

Projects

User does not belong to any projects.

User Details

User Since
May 5 2017, 1:55 PM (264 w, 7 h)

Recent Activity

Today

kiranchandramohan added a comment to D126471: [flang][OpenMP][OpenACC] Fix exit of a region.

I will fix the lowering later.

Fri, May 27, 1:51 AM · Restricted Project, Restricted Project
kiranchandramohan accepted D126368: [flang][OpenMP][NFC] Cleanup the sections tests.

LG.

Fri, May 27, 12:03 AM · Restricted Project, Restricted Project
kiranchandramohan accepted D126471: [flang][OpenMP][OpenACC] Fix exit of a region.

OK, LG.

Fri, May 27, 12:00 AM · Restricted Project, Restricted Project

Yesterday

kiranchandramohan added a comment to D126471: [flang][OpenMP][OpenACC] Fix exit of a region.

LG. Is the generated code valid?

Thu, May 26, 8:53 AM · Restricted Project, Restricted Project
kiranchandramohan added inline comments to D126375: [Flang][OpenMP] Fix for unstructured regions in OpenMP constructs - 2.
Thu, May 26, 7:57 AM · Restricted Project, Restricted Project
kiranchandramohan added a comment to D126368: [flang][OpenMP][NFC] Cleanup the sections tests.

Can you add one (atleast) test for sections in https://github.com/llvm/llvm-project/blob/main/flang/test/Fir/convert-to-llvm-openmp-and-fir.fir?

Thu, May 26, 5:06 AM · Restricted Project, Restricted Project
kiranchandramohan added a comment to D126368: [flang][OpenMP][NFC] Cleanup the sections tests.

BTW, can you create one more PR to fix all the tests under flang/Lower/OpenMP? Others will then follow the same style.

Thu, May 26, 5:04 AM · Restricted Project, Restricted Project
kiranchandramohan accepted D124610: [OpenMP] Support operation conversion to LLVM for threadprivate directive.

LGTM.

Thu, May 26, 4:06 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
kiranchandramohan added a comment to D126404: [mlir][OpenMP] Add recursive side effect information and tests for canonicalize pass.

Not sure about this change, particularly when there are no optimisations specified.

Thu, May 26, 3:20 AM · Restricted Project, Restricted Project, Restricted Project
kiranchandramohan added inline comments to D126375: [Flang][OpenMP] Fix for unstructured regions in OpenMP constructs - 2.
Thu, May 26, 1:37 AM · Restricted Project, Restricted Project

Wed, May 25

kiranchandramohan added a comment to D125788: [flang][driver] Rename `flang-new` as `flang`.

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.

Wed, May 25, 8:04 AM · Restricted Project, Restricted Project, Restricted Project
kiranchandramohan retitled D126375: [Flang][OpenMP] Fix for unstructured regions in OpenMP constructs - 2 from [OpenMP] Fix for unstructured regions in OpenMP constructs to [Flang][OpenMP] Fix for unstructured regions in OpenMP constructs - 2.
Wed, May 25, 6:27 AM · Restricted Project, Restricted Project
kiranchandramohan requested review of D126375: [Flang][OpenMP] Fix for unstructured regions in OpenMP constructs - 2.
Wed, May 25, 6:26 AM · Restricted Project, Restricted Project
kiranchandramohan added a comment to D126291: [flang][Driver] Update link job on windows.

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

Wed, May 25, 3:59 AM · Restricted Project, Restricted Project, Restricted Project
kiranchandramohan added a comment to D126272: [mlir][openmp] Add check for types of operands in omp.atomic.write.

I have two concerns for this:

subroutine atomic_write_pointer()
  integer, pointer :: x
  !$omp atomic write
    x = 1
end

For the above test case, without this patch, it reports the following:

$ flang-new -fopenmp write-atomic.f90 
flang-new: /home/.../llvm-project/llvm/lib/IR/Instructions.cpp:1510: void llvm::StoreInst::AssertOK(): Assertion `cast<PointerType>(getOperand(1)->getType()) ->isOpaqueOrPointeeTypeMatches(getOperand(0)->getType()) && "Ptr must be a pointer to Val type!"' failed.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.

Users will know this is the bug in flang, not their code problem. With this patch, it reports the following:

$ flang-new -fopenmp write-atomic.f90 
error: loc("./write-atomic.f90":1:1): address must dereference to value type
error: verification of lowering to FIR failed

Users will be confused if this is the problem of the compiler or their code. Does "address must dereference to value type" mean I should only use value instead of pointer? Can I only use integer :: x here?

  1. I don't know the purpose of mlir-opt. Does it test the operation conversion here? If yes, I think semantic analysis should not be covered here. The semantic analysis is the job in frontend. This is codegen.

This reminds me that some errors may should not be reported in mlir. The errors reporting will make users be confused if there is some bug in frontend. Maybe an assertion is better than an error.

Wed, May 25, 2:38 AM · Restricted Project, Restricted Project
kiranchandramohan added inline comments to D126293: [Flang][OpenMP] Fixes for unnstructured OpenMP code.
Wed, May 25, 2:11 AM · Restricted Project, Restricted Project
kiranchandramohan accepted D126272: [mlir][openmp] Add check for types of operands in omp.atomic.write.

Agree with @shraiysh. This is invalid MLIR and we should catch it at this layer.

Wed, May 25, 2:08 AM · Restricted Project, Restricted Project

Tue, May 24

kiranchandramohan committed rG29f167abcf7d: [Flang][OpenMP] Fixes for unstructured OpenMP code (authored by kiranchandramohan).
[Flang][OpenMP] Fixes for unstructured OpenMP code
Tue, May 24, 2:42 PM · Restricted Project, Restricted Project
kiranchandramohan closed D126293: [Flang][OpenMP] Fixes for unnstructured OpenMP code.
Tue, May 24, 2:42 PM · Restricted Project, Restricted Project
kiranchandramohan updated the diff for D126293: [Flang][OpenMP] Fixes for unnstructured OpenMP code.

Remove commented code block.

Tue, May 24, 2:31 PM · Restricted Project, Restricted Project
kiranchandramohan added inline comments to D126293: [Flang][OpenMP] Fixes for unnstructured OpenMP code.
Tue, May 24, 5:27 AM · Restricted Project, Restricted Project
kiranchandramohan requested review of D126293: [Flang][OpenMP] Fixes for unnstructured OpenMP code.
Tue, May 24, 5:20 AM · Restricted Project, Restricted Project
kiranchandramohan added reviewers for D126291: [flang][Driver] Update link job on windows: isuruf, Meinersbur.
Tue, May 24, 5:02 AM · Restricted Project, Restricted Project, Restricted Project

Mon, May 23

kiranchandramohan added a comment to D125891: [flang] Initial support for storage conflict check.

You will probably have to wait till WARNINGs support is enabled in test_errors.py (https://reviews.llvm.org/D125804).

Mon, May 23, 7:50 AM · Restricted Project, Restricted Project
kiranchandramohan added a comment to D126195: [mlir][OpenMP] Add memory_order clause tests.

Can you add a reason for this check, assuming it is specific for MLIR since the MLIR atomic capture operation can have another MLIR atomic operation inside?

Mon, May 23, 7:29 AM · Restricted Project, Restricted Project
kiranchandramohan added inline comments to D124610: [OpenMP] Support operation conversion to LLVM for threadprivate directive.
Mon, May 23, 6:35 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
kiranchandramohan added inline comments to D124610: [OpenMP] Support operation conversion to LLVM for threadprivate directive.
Mon, May 23, 6:07 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
kiranchandramohan added a comment to D124610: [OpenMP] Support operation conversion to LLVM for threadprivate directive.

I don't figure out why OpenACC uses the DataDescriptor for memref conversion. I tested the following test case in both fir-dev and upstream LLVM as follows:

Mon, May 23, 4:21 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
kiranchandramohan added a comment to D124156: [flang][OpenMP] Added parser support for in_reduction clause on OpenMP Task directive.

Does this need changes in flang/examples/FlangOmpReport/FlangOmpReportVisitor.cpp? @kiranchandramohan

LGTM. Please wait for @kiranchandramohan 's comments about this.

Mon, May 23, 2:46 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Thu, May 19

kiranchandramohan added inline comments to D125465: [Flang][OpenMP] Upstream the lowering of the parallel do combined construct.
Thu, May 19, 2:28 PM · Restricted Project, Restricted Project
kiranchandramohan accepted D125740: [flang][OpenMP] Fix the types of worksharing-loop variables.

LGTM.

Thu, May 19, 2:22 PM · Restricted Project, Restricted Project
kiranchandramohan committed rG4202d69d9efe: [Flang][OpenMP] Upstream the lowering of the parallel do combined construct (authored by kiranchandramohan).
[Flang][OpenMP] Upstream the lowering of the parallel do combined construct
Thu, May 19, 2:19 PM · Restricted Project, Restricted Project
kiranchandramohan closed D125465: [Flang][OpenMP] Upstream the lowering of the parallel do combined construct.
Thu, May 19, 2:19 PM · Restricted Project, Restricted Project
kiranchandramohan added inline comments to D125465: [Flang][OpenMP] Upstream the lowering of the parallel do combined construct.
Thu, May 19, 9:36 AM · Restricted Project, Restricted Project
kiranchandramohan updated the diff for D125465: [Flang][OpenMP] Upstream the lowering of the parallel do combined construct.

Refactor genProcBindKindAttr.

Thu, May 19, 7:42 AM · Restricted Project, Restricted Project
kiranchandramohan accepted D125967: [flang] Fix XArrayCoorOp conversion for index type slices.

Apologies, this was probably caused while switching from operands to converted operands in the upstream code.

Yes. That's what I also noticed. Anyway, using operands directly rather than converted operands in dir-dev is not correct.

Thu, May 19, 7:26 AM · Restricted Project, Restricted Project
kiranchandramohan accepted D125791: [flang] Fix use-associated false-positive error.

LGTM. Please wait a day before submitting in case others have comments or concerns.

Thu, May 19, 6:57 AM · Restricted Project, Restricted Project
kiranchandramohan added a comment to D125967: [flang] Fix XArrayCoorOp conversion for index type slices.

@jeanPerier @schweitz When do you plan to upstream the IO runtime functions to the libraries?

Thu, May 19, 6:01 AM · Restricted Project, Restricted Project
kiranchandramohan added a comment to D125788: [flang][driver] Rename `flang-new` as `flang`.

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.

Thu, May 19, 1:50 AM · Restricted Project, Restricted Project, Restricted Project

Wed, May 18

kiranchandramohan added a comment to D125788: [flang][driver] Rename `flang-new` as `flang`.

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)?

Wed, May 18, 12:57 PM · Restricted Project, Restricted Project, Restricted Project
kiranchandramohan added a comment to D125791: [flang] Fix use-associated false-positive error.

Can you add an error test to ensure that if the function foo is called then the Ambiguous error message is preserved?

Wed, May 18, 5:19 AM · Restricted Project, Restricted Project
kiranchandramohan added a comment to D125465: [Flang][OpenMP] Upstream the lowering of the parallel do combined construct.

How would this affect parallel sections? Should that be changed to use the new function createCombinedParallelOp too? If not, can we change the comment to state that this is for "loop constructs" only?

Wed, May 18, 4:12 AM · Restricted Project, Restricted Project
kiranchandramohan updated the diff for D125465: [Flang][OpenMP] Upstream the lowering of the parallel do combined construct.

Changes to address review comments.
-> Call the createCombinedParallelOp for Parallel sections too.
-> Add tests for privatisation clauses.
-> Modified the TODO

Wed, May 18, 4:11 AM · Restricted Project, Restricted Project
kiranchandramohan accepted D125788: [flang][driver] Rename `flang-new` as `flang`.

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.

Wed, May 18, 2:08 AM · Restricted Project, Restricted Project, Restricted Project

Tue, May 17

kiranchandramohan added inline comments to D125302: [flang][OpenMP] Support for Collapse.
Tue, May 17, 1:45 AM · Restricted Project, Restricted Project

Mon, May 16

kiranchandramohan accepted D125456: [flang][OpenMP] Support lowering to MLIR for ordered clause.

LGTM. Have one comment. Also please respond to @NimishMishra's comments.

Mon, May 16, 7:29 AM · Restricted Project, Restricted Project, Restricted Project

Fri, May 13

kiranchandramohan accepted D125007: [flang][driver] Switch to the MLIR coding style in the driver.

LGTM.

Fri, May 13, 6:40 AM · Restricted Project, Restricted Project
kiranchandramohan accepted D125302: [flang][OpenMP] Support for Collapse.

It seems you missed a comment. Otherwise LGTM.

Fri, May 13, 4:41 AM · Restricted Project, Restricted Project

Thu, May 12

kiranchandramohan requested review of D125465: [Flang][OpenMP] Upstream the lowering of the parallel do combined construct.
Thu, May 12, 6:10 AM · Restricted Project, Restricted Project

Tue, May 10

kiranchandramohan added inline comments to D125282: [Flang][OpenMP] Implementation of lowering of SIMD construct..
Tue, May 10, 2:07 PM · Restricted Project, Restricted Project
kiranchandramohan added a comment to D125302: [flang][OpenMP] Support for Collapse.

Looks OK. A few Nit comments.

Tue, May 10, 9:00 AM · Restricted Project, Restricted Project
kiranchandramohan added a comment to D125282: [Flang][OpenMP] Implementation of lowering of SIMD construct..

Looks OK. Have a few comments.

Tue, May 10, 6:57 AM · Restricted Project, Restricted Project

Mon, May 9

kiranchandramohan accepted D125211: [flang] Fix windows bot after D125140.

LGTM.

Mon, May 9, 4:26 AM · Restricted Project, Restricted Project

Fri, May 6

kiranchandramohan committed rGb85c39dd0078: [Flang][OpenMP] Initial lowering of the OpenMP worksharing loop (authored by kiranchandramohan).
[Flang][OpenMP] Initial lowering of the OpenMP worksharing loop
Fri, May 6, 5:01 AM · Restricted Project, Restricted Project
kiranchandramohan closed D125024: [Flang][OpenMP] Initial lowering of the OpenMP worksharing loop.
Fri, May 6, 5:00 AM · Restricted Project, Restricted Project
kiranchandramohan accepted D124914: [flang] Fix internal error with DATA-statement style initializers.

LGTM. Thanks.

Fri, May 6, 3:55 AM · Restricted Project, Restricted Project
kiranchandramohan added inline comments to D125024: [Flang][OpenMP] Initial lowering of the OpenMP worksharing loop.
Fri, May 6, 3:16 AM · Restricted Project, Restricted Project
kiranchandramohan updated the diff for D125024: [Flang][OpenMP] Initial lowering of the OpenMP worksharing loop.

Address review comments from @peixin.

Fri, May 6, 3:11 AM · Restricted Project, Restricted Project
kiranchandramohan committed rGaa0e167fab88: [Flang] Lower Unstructured do loops (authored by kiranchandramohan).
[Flang] Lower Unstructured do loops
Fri, May 6, 2:20 AM · Restricted Project, Restricted Project
kiranchandramohan closed D124837: [Flang] Lower Unstructured do loops.
Fri, May 6, 2:19 AM · Restricted Project, Restricted Project
kiranchandramohan added a comment to D124914: [flang] Fix internal error with DATA-statement style initializers.

Apologies for the confusion with the reviewers. Please feel free to add reviewers from this list. Depending on time, and familiarity with the area one or more will respond. Thanks for the contribution.

Fri, May 6, 1:57 AM · Restricted Project, Restricted Project

Thu, May 5

kiranchandramohan updated the summary of D125024: [Flang][OpenMP] Initial lowering of the OpenMP worksharing loop.
Thu, May 5, 11:21 AM · Restricted Project, Restricted Project
kiranchandramohan requested review of D125024: [Flang][OpenMP] Initial lowering of the OpenMP worksharing loop.
Thu, May 5, 10:42 AM · Restricted Project, Restricted Project

Wed, May 4

kiranchandramohan updated the diff for D124837: [Flang] Lower Unstructured do loops.

Addressed comments from @awarzynski. Sure will wait before merging.

Wed, May 4, 10:28 AM · Restricted Project, Restricted Project
kiranchandramohan requested changes to D124914: [flang] Fix internal error with DATA-statement style initializers.

For parser, semantics, runtime patches please add @klausler as the primary reviewer.

Wed, May 4, 4:43 AM · Restricted Project, Restricted Project

Tue, May 3

kiranchandramohan requested review of D124837: [Flang] Lower Unstructured do loops.
Tue, May 3, 3:38 AM · Restricted Project, Restricted Project

Sat, Apr 30

kiranchandramohan accepted D124142: [mlir][OpenMP] Restrict types for omp.parallel args.

LGTM.
You may have some of the tests for num_threads with index, i64 etc.

Sat, Apr 30, 11:28 PM · Restricted Project, Restricted Project, Restricted Project
kiranchandramohan accepted D124229: [flang] Added tests for taskwait and taskyield translation.

LGTM.

Sat, Apr 30, 11:21 PM · Restricted Project, Restricted Project

Fri, Apr 29

kiranchandramohan added a comment to D124610: [OpenMP] Support operation conversion to LLVM for threadprivate directive.

I think this change in MLIR is better checked in MLIR itself by writing a conversion for the memref type similar to what is done in OpenACC (https://github.com/llvm/llvm-project/blob/05b0a498329c4b5db367120e5c9358bb74346131/mlir/lib/Conversion/OpenACCToLLVM/OpenACCToLLVM.cpp#L100)

Note that the memref type is already added to the OpenMP Pointer model accepted by threadprivate and atomic operations.
https://github.com/llvm/llvm-project/blob/05b0a498329c4b5db367120e5c9358bb74346131/mlir/lib/Dialect/OpenMP/IR/OpenMPDialect.cpp#L59

  1. The function code in this patch can support all Fortran data types conversion. The curOp.getType() for integer pointer in Fortran is !fir.ref<!fir.box<!fir.ptr<i32>>>, which will be converted into !llvm.ptr<struct<(ptr<i32>, i64, i32, i8, i8, i8, i8)>> by the ConvertToLLVMPattern::getTypeConverter(). Why do we want to support the memref type here? Is that because the memref type is the common type in MLIR and can be used not only for Fortran?

Yes, memref is the only in-tree MLIR dialect data type for which the Conversion/OpenMPToLLVM apply, in the case of threadprivate and atomic ops. memref needs some special handling because it converts to a struct and not a pointer and threadprivate/atomic ops do not accept that.

Fri, Apr 29, 2:08 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project

Thu, Apr 28

kiranchandramohan added a comment to D124142: [mlir][OpenMP] Restrict types for omp.parallel args.

Any reason to restrict num_threads to be 32-bit specifically? The spec says it must evaluate to a positive integer value, nothing about its size. I would rather keep it as AnyInteger, and maybe even allowed Index.

Thu, Apr 28, 7:57 AM · Restricted Project, Restricted Project, Restricted Project
kiranchandramohan requested changes to D124610: [OpenMP] Support operation conversion to LLVM for threadprivate directive.

I think this change in MLIR is better checked in MLIR itself by writing a conversion for the memref type similar to what is done in OpenACC (https://github.com/llvm/llvm-project/blob/05b0a498329c4b5db367120e5c9358bb74346131/mlir/lib/Conversion/OpenACCToLLVM/OpenACCToLLVM.cpp#L100)

Thu, Apr 28, 6:31 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
kiranchandramohan added a comment to D124277: [Flang] Initial lowering of the Fortran Do loop.

Changes made while submitting.

Thu, Apr 28, 6:08 AM · Restricted Project, Restricted Project
kiranchandramohan committed rGb5b3e50f65ee: [Flang] Initial lowering of the Fortran Do loop (authored by kiranchandramohan).
[Flang] Initial lowering of the Fortran Do loop
Thu, Apr 28, 6:07 AM · Restricted Project, Restricted Project
kiranchandramohan closed D124277: [Flang] Initial lowering of the Fortran Do loop.
Thu, Apr 28, 6:07 AM · Restricted Project, Restricted Project
kiranchandramohan added inline comments to D124190: [flang][OpenMP] Added parser support for defaultmap (OpenMP 5.0).
Thu, Apr 28, 4:08 AM · Restricted Project
kiranchandramohan accepted D123828: [mlir][OpenMP] Add omp.cancel and omp.cancellationpoint..

LGTM. Please check the CI failure (probably a clang-format issue). Please wait for @shraiysh & @peixin.

Thu, Apr 28, 4:05 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project
kiranchandramohan accepted D124190: [flang][OpenMP] Added parser support for defaultmap (OpenMP 5.0).

Thanks Dossay for the contribution. LGTM with the two Nits suggested and CI passing. Please wait for @shraiysh and @peixin to approve.

Thu, Apr 28, 2:19 AM · Restricted Project

Apr 27 2022

kiranchandramohan updated the diff for D124523: [Flang][WIP] Dummy test to trigger Windows CI.

Retrigger CI

Apr 27 2022, 7:24 AM · Restricted Project, Restricted Project
kiranchandramohan updated the summary of D124523: [Flang][WIP] Dummy test to trigger Windows CI.
Apr 27 2022, 7:13 AM · Restricted Project, Restricted Project
kiranchandramohan requested review of D124523: [Flang][WIP] Dummy test to trigger Windows CI.
Apr 27 2022, 5:42 AM · Restricted Project, Restricted Project
kiranchandramohan committed rGacd75440c67a: [Flang] Lower the FailImage Statement (authored by kiranchandramohan).
[Flang] Lower the FailImage Statement
Apr 27 2022, 5:25 AM · Restricted Project, Restricted Project
kiranchandramohan closed D124520: [Flang] Lower the FailImage Statement.
Apr 27 2022, 5:25 AM · Restricted Project, Restricted Project
kiranchandramohan accepted D124225: [flang] Add lowering stubs for OpenMP/OpenACC declarative constructs.

We have a facility for checking TODO messages. See https://reviews.llvm.org/D114371. If you are interested, you can try this.

Apr 27 2022, 5:00 AM · Restricted Project, Restricted Project
kiranchandramohan requested review of D124520: [Flang] Lower the FailImage Statement.
Apr 27 2022, 4:37 AM · Restricted Project, Restricted Project
kiranchandramohan added a comment to D124226: [flang][OpenMP] Support lowering parse-tree to MLIR for threadprivate directive.

Even we can support integration test here, threadprivate-pointer-allocatable.f90 still needs some patches upstreamed to take the integration test. Also, this patch blocks the review for the lowering of default clause and the development of the copyin clause. Considering the type conversion things has not been fully upstreamed, can I change this patch to only support lowering parse-tree to MLIR for threadprivate? At that time, we can follow the style of what OpenACC does.

Apr 27 2022, 2:37 AM · Restricted Project, Restricted Project, Restricted Project
kiranchandramohan added a comment to D124226: [flang][OpenMP] Support lowering parse-tree to MLIR for threadprivate directive.

There are some integration tests in MLIR that seems to be switched OFF by default and only enabled by building with a CMake option. There is probably a CI somewhere testing it. This could possibly be an option for us.
https://discourse.llvm.org/t/vectorops-rfc-add-suite-of-integration-tests-for-vector-dialect-operations/1213

Apr 27 2022, 2:15 AM · Restricted Project, Restricted Project, Restricted Project
kiranchandramohan added a comment to D124226: [flang][OpenMP] Support lowering parse-tree to MLIR for threadprivate directive.
OpenMP Dialect + FIR-Dialect  -> OpenMP Dialect + LLVM Dialect
 (flang side, FIR data type)        (mlir side, LLVM data type)

BTW, these two OpenMP Dialect have minor differences, mainly data type? Currently, this translation is tested in flang side by using tco or fir-opt.

Apr 27 2022, 1:55 AM · Restricted Project, Restricted Project, Restricted Project
kiranchandramohan added a comment to D124226: [flang][OpenMP] Support lowering parse-tree to MLIR for threadprivate directive.

It is not. As we can see, the test for atomic read/write and threadprivate are sucessful in openmp-llvm.mlir without changes in OpenMPToLLVM.cpp. The code in mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp needs to be tested by tco or fir-opt, which are both under flang. For example, the code is tested for atomic read/write using fir-opt in D122725.

Hmm, this is a strange situation, the tests for changes in mlir/ should reside under mlir/ and not under flang/. I think we can add tests for such changes by adding tests for these while converting OpenMP Dialect + X-dialect (any other dialect) to OpenMP Dialect + LLVM Dialect in mlir/. @rriddle @ftynse what would be the best place/combination of dialects for testing the code in mlir/lib/Conversion/OpenMPToLLVM/OpenMPToLLVM.cpp?

Apr 27 2022, 1:26 AM · Restricted Project, Restricted Project, Restricted Project

Apr 26 2022

kiranchandramohan added a comment to D124277: [Flang] Initial lowering of the Fortran Do loop.

@clementval @schweitz Plan is to upstream the loop as a sequence of patches (if possible without code changes). Since the first patch (this one) only brings in a simple loop, the tests in fir-dev were not directly applicable and I decided to write a new one. Since it is a new test file, its name and contents do not matter much.
Once the loop is completely upstreamed, I will upstream all the remaining loop tests if they did not make it along with the code patches.

Apr 26 2022, 10:21 AM · Restricted Project, Restricted Project
kiranchandramohan added a comment to D124277: [Flang] Initial lowering of the Fortran Do loop.

Thanks @awarzynski for the review comments. The CHECK lines are mostly mechanical and I have tried to keep it all simple (all except one do not have a loop body). The pattern is something like the following, this is probably evident from the first test (simple_loop).
-> there are some constants/variables that are converted to index type. these form the start, end and step values of the loop
-> the fir.do_loop
-> store the mlir value of the index to the loop variable
-> compute the next value of the index and return or pass it to the next iteration
-> outside the loop the final/result value of the index is stored to the loop variable

Apr 26 2022, 5:17 AM · Restricted Project, Restricted Project
kiranchandramohan updated the diff for D124277: [Flang] Initial lowering of the Fortran Do loop.

Address @awarzynksi's comments regarding better tests.

Apr 26 2022, 5:02 AM · Restricted Project, Restricted Project
kiranchandramohan added a comment to D124226: [flang][OpenMP] Support lowering parse-tree to MLIR for threadprivate directive.

Is there any issue with long file names as long as they are descriptive but not complete sentences. On the another hand, can we add all the relevant tests in one file, and separate them using comments describing each of them individually? (like loop.f90 in D124277).

Any of them is OK to me. I guess there is no strict rule for adding test cases. Usually, I followed one of the previous provided test cases.

Also, the tests for this minor change are huge. This is primarily because these tests try to check four things at once - thus making it integration/regression test instead of unit test. A test failure in such a scenario does not help us much. A suggestion - can we please divide the tests into individual stages so that they are more targeted and provide helpful feedback in case of failures:

flang/test/Lower/OpenMP/*.f90 - check generation of FIR and OpenMPDialect only (these tests would be added in this patch)
flang/test/Fir/convert-to-llvm.fir - check FIR to LLVM IR Dialect only.
mlir/test/Target/LLVMIR/llvmir.mlir - Check LLVM IR Dialect to LLVM IR
mlir/test/Target/LLVMIR/openmp-llvm.mlir - check OpenMP Dialect to LLVM IR
Only the first kind of tests are required to be added in this patch, and maybe upto LLVM Dialect, but beyond that, it just makes the test lengthy and hard to understand. We can however discuss about having some integration tests separate from unit tests (am open to ideas about this).

You propose one good question here. When I join the OpenMP development group, I was told to follow the following process:

  1. OMPIRBuilder: unittest and IR check for clang
  2. MLIR Op Def: check dialect
  3. lowering from MLIR to LLVMIR: check MLIR to LLVMIR
  4. lowering from parse-tree to MLIR: check end-to-end (was using bbc and tco)

Is there one rule to add the test cases? @kiranchandramohan

I think there is benefit to check end-to-end in step 4. When I develop the code for ordered clause, I found several bugs about work-sharing loop lowering since the end-to-end testing is missed. Usually, the MLIR check cannot have full combination tests, especially for data-related constructs/clauses. Of course, we can add them all, but it takes much more time to add the test cases there. I guess checking end-to-end was in step 4 using bbc and tco since it is the least time-consuming work.

Apr 26 2022, 3:51 AM · Restricted Project, Restricted Project, Restricted Project
kiranchandramohan added a comment to D124423: [flang] Get ppc64le build bot back up.

The CI is complaining about a clang-format issue in flang/lib/Evaluate/intrinsics-library.cpp

Apr 26 2022, 3:09 AM · Restricted Project, Restricted Project
kiranchandramohan accepted D118409: [OpenMPIRBuilder] Remove ContinuationBB argument from Body callback..

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

Apr 26 2022, 2:53 AM · Restricted Project, Restricted Project, Restricted Project, Restricted Project, Restricted Project

Apr 25 2022

kiranchandramohan added a comment to D103723: [MLIR][OpenMP]Add custom parser and pretty printer for target construct.

You can update this patch itself here.

Apr 25 2022, 1:41 PM · Restricted Project, Restricted Project
kiranchandramohan requested changes to D103723: [MLIR][OpenMP]Add custom parser and pretty printer for target construct.

Unfortunately, there have been several developments since we last looked at this patch. This includes using assembly format for the printer/parser instead of the custom printTargetOp/parseTargetOp. See the following for reference.
https://github.com/llvm/llvm-project/blob/6f73bd781305266a747055875ce8352e5a36c809/mlir/include/mlir/Dialect/OpenMP/OpenMPOps.td#L115

Apr 25 2022, 10:20 AM · Restricted Project, Restricted Project
kiranchandramohan added reviewers for D103723: [MLIR][OpenMP]Add custom parser and pretty printer for target construct: shraiysh, peixin.
Apr 25 2022, 10:09 AM · Restricted Project, Restricted Project
kiranchandramohan added a comment to D124366: [mlir][vector] insert `alloca`s outside of loops.

We should add a facility (a pass presumably) that hoists allocas up to some user-specified level. Maybe the allocation scope that is not nested in another allocation scope, but is nested in the closest isolated-from-above operation. I believe @wsmoses has a prototype for this.

Apr 25 2022, 6:22 AM · Restricted Project, Restricted Project
kiranchandramohan accepted D123947: [flang] Fix ICE for passing a label for non alternate return arguments.

LGTM. Thanks for this patch. In case there are comments from other reviewers, please wait for a day before you submit.

Apr 25 2022, 5:27 AM · Restricted Project, Restricted Project
kiranchandramohan added reviewers for D124190: [flang][OpenMP] Added parser support for defaultmap (OpenMP 5.0): peixin, shraiysh, NimishMishra.
Apr 25 2022, 5:24 AM · Restricted Project