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

peixin added a comment. @Meinersbur Thanks for your review and suggestions. > While manually editing `ordered_codegen.cpp` gives nicer results, re-running > `update_cc_test_checks.py` would be less work. Your manual changes would be > also be dropped the next time somebody runs

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

peixin added a comment. @Meinersbur Thanks a lot for your review and accepting this patch. BTW, I finished the implementation of the codegen of ordered construct for LLVM Flang and the OpenMP IRBuilder of ordered construct in this patch also works well for LLVM Flang. Is it OK to land this

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

peixin added a comment. Here is the PR for codegen of ordered construct for LLVM Flang: https://github.com/flang-compiler/f18-llvm-project/pull/1027. I create the PR on fir-dev branch since the test of lowering parse-tree to MLIR for LLVM Flang is still not supported in upstream llvm-project.

[PATCH] D109321: [clang][OpenMP] Fix the bug in codegen for ordered directive

peixin created this revision. peixin added reviewers: jhuber6, jdoerfert, kiranchandramohan, Meinersbur, clementval, kiranktp, Leporacanthicus, bryanpkc, arnamoy10, Chuanfeng. peixin added projects: LLVM, clang. Herald added subscribers: guansong, yaxunl. peixin requested review of this revision.

[PATCH] D109321: [clang][OpenMP] Fix the bug in codegen for ordered directive

peixin added a comment. In D109321#2987783 , @jdoerfert wrote: > As noted before, this is not the right fix. Not inlining should not cause a > semantic difference here, the problem is something else. @jdoerfert Yes, I agree that this is not the right

[PATCH] D109321: [clang][OpenMP] Fix the bug in codegen for ordered directive

peixin added a comment. The following test case fails after https://reviews.llvm.org/rGaf000197c4214926bd7d0862d86f89aed5f20da6. #include using namespace std; int main() { float a[10]; int i, N = 10; for (i = 0; i < N; i++) a[i] = 0; #pragma omp simd for

[PATCH] D109321: [clang][OpenMP] Fix the bug in codegen for ordered directive

peixin added a comment. In D109321#2985284 , @jhuber6 wrote: > In D109321#2985281 , @lebedev.ri > wrote: > >> Aha. But i don't think this is the right fix, >> the fact that the inlining manifests the miscompile

[PATCH] D109321: [clang][OpenMP] Fix the bug in codegen for ordered directive

peixin added a comment. `for (i = 0; i < N; i++)` --> `for (i = 1; i < N; i++)` #include using namespace std; int main() { float a[10]; int i, N = 10; for (i = 0; i < N; i++) a[i] = 0; #pragma omp simd for (i = 1; i < N; i++) { #pragma omp ordered

[PATCH] D109321: [clang][OpenMP] Fix the bug in codegen for ordered directive

peixin abandoned this revision. peixin added a comment. Abandon this PR since this is not right fix. Continuing discussion will be on openmp-dev mail list. This bug should be fixed if `ordered simd` is processed correctly in frontend and vectorization pass. Repository: rG LLVM Github

[PATCH] D109321: [clang][OpenMP] Fix the bug in codegen for ordered directive

peixin added a comment. $ clang++ -fopenmp simd.cpp -O1 -Xclang -disable-llvm-passes && ./a.out 0 1 2 3 4 5 6 7 8 9 $ clang++ -fopenmp simd.cpp -O2 && ./a.out 0 1 2 3 4 5 6 7 8 9 $ clang++ -fopenmp simd.cpp -O3 && ./a.out 0 1 2 3 4 5 6 7 8 9 This bug is not in

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

peixin updated this revision to Diff 365170. peixin added a comment. Revise the implementation by not generating "kmp_ordered" runtime functions when -fopenmp-enable-irbuilder is not enabled, which is consistent with that not using OpenMPIRBuilder. CHANGES SINCE LAST ACTION

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

peixin added a comment. @kiranchandramohan Thanks for the review. Removed the OpenMP 1.0 target from the description. The simd clause does need more changes and I think it also depend simd directive in the OpenMPIRBuilder. According to the CHECK5 in the test case `ordered_codegen.cpp`, it

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

peixin updated this revision to Diff 364389. peixin added a comment. @Meinersbur Thanks for the review. Removed the typo fixes and gave the alloca register one name. Also found some typos of invalid case style for variable 'cur' in llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp and will fix

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

peixin added a comment. In D114413#3166979 , @Meinersbur wrote: > In D114413#3160044 , @peixin wrote: > >> In D114413#3159095 , @Meinersbur >> wrote: >> >>> In

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

peixin added a comment. @Meinersbur Please rebase on main. The function "getPreheader()" was moved into OMPIRBuilder.h. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114413/new/ https://reviews.llvm.org/D114413

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

peixin added inline comments. Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:5993 case OMPC_adjust_args: + case OMPC_memory_order: llvm_unreachable("Clause is not allowed in 'omp atomic'."); The memory_order clause in clang side is not handled in this

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

peixin added inline comments. Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:5993 case OMPC_adjust_args: + case OMPC_memory_order: llvm_unreachable("Clause is not allowed in 'omp atomic'."); peixin wrote: > The memory_order clause in clang side is not

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

peixin added a comment. Can you check the following example by applying this patch on fir-dev? program main integer :: i, N = 10 real :: x = 0 !$omp do schedule(static, 2) do i = 3, N x = x + i end do !$omp end do print *, x end Test running result:

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

peixin added a comment. In D114413#3159095 , @Meinersbur wrote: > In D114413#3154936 , @peixin wrote: > >> @Meinersbur Please rebase on main. The function "getPreheader()" was moved >> into OMPIRBuilder.h. > >

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

peixin added a comment. Herald added a subscriber: awarzynski. @Meinersbur Here is the c++ code test. Without the chunk size specified, the running result using OMPIRBuilder is correct. #include int main() { int i, N = 10; float x = 0.0; #pragma omp for schedule(static, 2)

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

peixin added a comment. BTW, please rebase on main. There is one conflict about function `getOrCreateSrcLocStr`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114413/new/ https://reviews.llvm.org/D114413

[PATCH] D121101: [clang] Fix OpenMP critical hint parameter check

peixin created this revision. peixin added a reviewer: ABataev. peixin added a project: OpenMP. Herald added subscribers: guansong, yaxunl. Herald added a project: All. peixin requested review of this revision. Herald added a reviewer: jdoerfert. Herald added subscribers: cfe-commits, sstefan1.

[PATCH] D121101: [clang] Fix OpenMP critical hint parameter check

This revision was automatically updated to reflect the committed changes. Closed by commit rG4e159e4c7b97: [clang] Fix OpenMP critical hint parameter check (authored by peixin). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D121101/new/

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

peixin added inline comments. Herald added a project: All. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1505 + /// valid in the condition block (i.e., defined in the preheader) and is + /// interpreted as an unsigned integer. + void setTripCount(Value

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

peixin added a comment. Except for three nits. LGTM. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:1505 + /// valid in the condition block (i.e., defined in the preheader) and is + /// interpreted as an unsigned integer. + void setTripCount(Value *TripCount);

[PATCH] D117835: [OpenMPIRBuilder] Detect and fix ambiguous InsertPoints for createSections.

peixin added a comment. Added @shraiysh and @NimishMishra . They may be more familiar to this code. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D117835/new/ https://reviews.llvm.org/D117835 ___

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

peixin added a comment. Thanks for the fix. The fix of off-by-one issue looks ok to me. Will continue reviewing other parts in one week due to the Spring Festival in China. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114413/new/

[PATCH] D118542: [Clang][OpenMPIRBuilder] Fix off-by-one error when dividing by stepsize.

peixin accepted this revision. peixin added a comment. This revision is now accepted and ready to land. Function code LGTM. Please fix the CI fail. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D118542/new/ https://reviews.llvm.org/D118542

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

peixin added a comment. When I investigated the edge cases you mentioned in D116292 . Found one unsupported case as follows #include #include using namespace std; void func(unsigned long long lb, unsigned long long ub, unsigned long long step) {

[PATCH] D114413: [OpenMPIRBuilder] Implement static-chunked workshare-loop schedules.

peixin added a comment. Can you rebase this? I cannot apply this patch on current main branch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114413/new/ https://reviews.llvm.org/D114413 ___ cfe-commits

[PATCH] D123403: [OpenMP] Refactor OMPScheduleType enum.

peixin added a comment. A few comments. Mostly nits. Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3767 + /*HasMonotonicModifier=*/false, /*HasNonmonotonicModifier=*/false, + /*HasOrdedClause=*/false); return;

[PATCH] D123403: [OpenMP] Refactor OMPScheduleType enum.

peixin accepted this revision. peixin added inline comments. This revision is now accepted and ready to land. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:135 + UnorderedGuidedSimd = BaseGuidedSimd | ModifierUnordered, // (46) + UnorderedRuntimeSimd =

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

peixin 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

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

peixin added a comment. In summary: > If you want to use the updated name, flang, set FLANG_USE_LEGACY_NAME to ON > when configuring LLVM Flang. OFF? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125788/new/ https://reviews.llvm.org/D125788

[PATCH] D128043: [flang][driver] Add support for `-O{0|1|2|3}`

peixin accepted this revision. peixin added a comment. This revision is now accepted and ready to land. LGTM Comment at: clang/include/clang/Driver/Options.td:732 +def O_flag : Flag<["-"], "O">, Flags<[CC1Option,FC1Option]>, Alias, AliasArgs<["1"]>; def Ofast : Joined<["-"],

[PATCH] D128043: [flang][driver] Add support for `-O{0|1|2|3}`

peixin added a comment. LGTM Comment at: clang/lib/Driver/ToolChains/Flang.cpp:133 + CmdArgs.push_back("-O3"); + TC.getDriver().Diag(diag::warn_O4_is_O3); +} else { Nit: I have committed D126164, and you can rebase and use D directly, which is

[PATCH] D126164: [flang][Driver] Refine _when_ driver diagnostics are formatted

This revision was automatically updated to reflect the committed changes. Closed by commit rG430841605d49: [flang][Driver] Refine _when_ driver diagnostics are formatted (authored by peixin). Herald added a project: clang. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github

[PATCH] D128043: [flang][driver] Add support for `-O{0|1|2|3}`

peixin added a comment. The CI failed. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128043/new/ https://reviews.llvm.org/D128043 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D128043: [flang][driver] Add support for `-O{0|1|2|3}`

peixin added a comment. OK. This seems not to be related to this patch. https://buildkite.com/llvm-project/premerge-checks/builds/98446#0181715d-812b-4d2a-b5d0-5c1283d78b5f I also see these failures in another review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D128043: [flang][driver] Add support for `-O{0|1|2|3}`

peixin added a comment. Thanks for this patch. This is one big progress to use `flang-new` for real workloads. Mostly looks good to me. I have several minor comments. If supporing `-Ofast` requires more changes, I prefer to push forward with the current patch. summary: fronted -> frontend

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

peixin added inline comments. Comment at: clang/include/clang/Driver/Options.td:4897 def ffixed_line_length_VALUE : Joined<["-"], "ffixed-line-length-">, Group, Alias; +def fconvert_EQ : Joined<["-"], "fconvert=">, Group, + HelpText<"Set endian conversion of data for

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

peixin added a comment. Currently, I cannot test the option `-fconvert=big-endian` with open statement with convert argument using the following code since lowering is not supported. I am not sure if it can be tested in runtime. $ cat fconvert_option_openfile.f90 module

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

peixin added a comment. This needs rebase. I failed to apply this patch due to conflict. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130513/new/ https://reviews.llvm.org/D130513 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

peixin added a comment. $ cat fconvert_option_main_2.f90 ! ! 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: Apache-2.0 WITH LLVM-exception ! ! checking for I/O:

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

peixin added inline comments. Comment at: flang/runtime/environment.cpp:77 +#else + envp = environ; +#endif What is `environ` used for? Should we try to keep as less extern variable as possible in runtime for security. Comment at:

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

peixin added a comment. In D130513#3804659 , @jpenix-quic wrote: >> The build still fails. > > I think it might be an infrastructure issue or something like that--I've > tried restarting it a few times and each time it just ends with "HTTP 28" as >

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

peixin added a comment. The build still fails. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130513/new/ https://reviews.llvm.org/D130513 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

peixin added a comment. LGTM. Tested on one little-endian machine with our internal tests (I attached them in the issue https://github.com/llvm/llvm-project/issues/55961) for the priority of `-fconvert=` option and `CONVERT` argument in open statement and all passed. I didn't see the the

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

peixin added inline comments. Comment at: flang/test/Driver/emit-mlir.f90:16 ! CHECK-NEXT: } +! CHECK-NEXT: fir.global @_QQEnvironmentDefaults constant : !fir.ref, !fir.ref> { +! CHECK-NEXT: %[[VAL_0:.*]] = fir.zero_bits !fir.ref, !fir.ref>

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

peixin added inline comments. Comment at: flang/runtime/environment.cpp:77 +#else + envp = environ; +#endif jpenix-quic wrote: > peixin wrote: > > What is `environ` used for? Should we try to keep as less extern variable > > as possible in runtime for

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

peixin added inline comments. Comment at: flang/test/Driver/emit-mlir.f90:16 ! CHECK-NEXT: } +! CHECK-NEXT: fir.global @_QQEnvironmentDefaults constant : !fir.ref, !fir.ref> { +! CHECK-NEXT: %[[VAL_0:.*]] = fir.zero_bits !fir.ref, !fir.ref>

[PATCH] D134821: [flang][driver] Allow main program to be in an archive

peixin added a comment. Please fix the clang-format issue. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134821/new/ https://reviews.llvm.org/D134821 ___ cfe-commits mailing list

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

peixin added a comment. Mostly LGTM. Have several more comments. Comment at: flang/lib/Lower/Bridge.cpp:279 +// are compiled separately. +if (hasMainProgram) { + createGlobalOutsideOfFunctionLowering([&]() { Nit Comment at:

[PATCH] D131808: [clang,flang] Add missing options fsyntax-only in help

peixin added a comment. The CI failed since the test cases in flang driver have new output. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131808/new/ https://reviews.llvm.org/D131808 ___ cfe-commits

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

peixin added inline comments. Comment at: flang/lib/Lower/Bridge.cpp:203 //them. for (Fortran::lower::pft::Program::Units : pft.getUnits()) { std::visit(Fortran::common::visitors{ Doing this can avoid add one variable to the bridge.

[PATCH] D130513: [Flang] Add -fconvert option to swap endianness for unformatted files

peixin added a comment. This needs rebase. Comment at: flang/lib/Lower/Bridge.cpp:203 //them. for (Fortran::lower::pft::Program::Units : pft.getUnits()) { std::visit(Fortran::common::visitors{ jpenix-quic wrote: > peixin wrote: > > Doing

[PATCH] D134821: [flang][driver] Allow main program to be in an archive

peixin added a comment. > @peixin , wdyt? @awarzynski Hi Andrzej, sorry for the late reply. I am distracted by several internal projects and other things in my life recently (just came back from vacation). My boss has not decided to let me continue working on LLVM Flang this year, yet.