Page MenuHomePhabricator

peixin (Peixin Qiao)
User

Projects

User does not belong to any projects.

User Details

User Since
Jul 12 2021, 6:52 PM (10 w, 3 d)

Recent Activity

Sat, Sep 18

peixin updated the diff for D110015: [MLIR][OpenMP] Add support for ordered construct.

Fix clang-tidy warning.

Sat, Sep 18, 9:27 PM · Restricted Project, Restricted Project

Fri, Sep 17

peixin requested review of D110015: [MLIR][OpenMP] Add support for ordered construct.
Fri, Sep 17, 6:32 PM · Restricted Project, Restricted Project
peixin committed rG6fb01a94708f: [flang][OpenMP] Add semantic checks for ordered construct (authored by peixin).
[flang][OpenMP] Add semantic checks for ordered construct
Fri, Sep 17, 6:54 AM
peixin closed D108512: [flang][OpenMP] Add semantic checks for ordered construct.
Fri, Sep 17, 6:54 AM · Restricted Project

Thu, Sep 16

peixin added a comment to D108512: [flang][OpenMP] Add semantic checks for ordered construct.

@kiranchandramohan Can I land this patch now?

Thu, Sep 16, 6:00 PM · Restricted Project

Wed, Sep 15

peixin requested review of D109864: [flang][OpenMP] Add semantic checks for threadprivate and declare target directives.
Wed, Sep 15, 6:39 PM · Restricted Project
peixin added a comment to D108512: [flang][OpenMP] Add semantic checks for ordered construct.

@kiranchandramohan Thanks for your review.

Thanks for this work. Please submit with @sameeranjoshi as co-author. Please wait a day to see whether there are any other comments.

I have added @sameeranjoshi as co-author in summary description and will do as you suggest.

Wed, Sep 15, 1:37 AM · Restricted Project
peixin updated the diff for D108512: [flang][OpenMP] Add semantic checks for ordered construct.

Rebase this patch.

Wed, Sep 15, 1:27 AM · Restricted Project

Tue, Sep 14

peixin committed rG268521218434: [flang][OpenMP] Add semantic check for threadprivate directive (authored by peixin).
[flang][OpenMP] Add semantic check for threadprivate directive
Tue, Sep 14, 9:26 AM
peixin closed D109685: [flang][OpenMP] Add semantic check for threadprivate directive.
Tue, Sep 14, 9:26 AM · Restricted Project
peixin added a comment to D109685: [flang][OpenMP] Add semantic check for threadprivate directive.

@kiranchandramohan Does this patch need more review or is it ok to land?

Tue, Sep 14, 6:23 AM · Restricted Project

Mon, Sep 13

peixin added a comment to D109685: [flang][OpenMP] Add semantic check for threadprivate directive.

LGTM. I have some Nit comments. Please fix it if possible.

What about the other checks?

Mon, Sep 13, 7:35 PM · Restricted Project
peixin updated the diff for D109685: [flang][OpenMP] Add semantic check for threadprivate directive.
Mon, Sep 13, 7:23 PM · Restricted Project
peixin requested review of D109685: [flang][OpenMP] Add semantic check for threadprivate directive.
Mon, Sep 13, 6:22 AM · Restricted Project

Thu, Sep 9

peixin abandoned D109321: [clang][OpenMP] Fix the bug in codegen for ordered directive.

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.

Thu, Sep 9, 6:07 PM · Restricted Project, Restricted Project

Wed, Sep 8

peixin added a comment to D109321: [clang][OpenMP] Fix the bug in codegen for ordered directive.

As noted before, this is not the right fix. Not inlining should not cause a semantic difference here, the problem is something else.

Wed, Sep 8, 1:02 AM · Restricted Project, Restricted Project

Tue, Sep 7

peixin added a comment to D109321: [clang][OpenMP] Fix the bug in codegen for ordered directive.
$ 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 clang frontend. I will post it in bugzilla.

Tue, Sep 7, 6:30 AM · Restricted Project, Restricted Project

Mon, Sep 6

peixin added a comment to D109321: [clang][OpenMP] Fix the bug in codegen for ordered directive.

Aha. But i don't think this is the right fix,
the fact that the inlining manifests the miscompile is a symptom.

Preventing the outlined region from being inlined would also hurt OpenMP performance considerably.

Mon, Sep 6, 7:08 AM · Restricted Project, Restricted Project
peixin added a comment to D109321: [clang][OpenMP] Fix the bug in codegen for ordered directive.

for (i = 0; i < N; i++) --> for (i = 1; i < N; i++)

#include <iostream>
using namespace std;
Mon, Sep 6, 6:42 AM · Restricted Project, Restricted Project
peixin added a comment to D109321: [clang][OpenMP] Fix the bug in codegen for ordered directive.

The following test case fails after https://reviews.llvm.org/rGaf000197c4214926bd7d0862d86f89aed5f20da6.

#include <iostream>
using namespace std;
Mon, Sep 6, 6:31 AM · Restricted Project, Restricted Project
peixin requested review of D109321: [clang][OpenMP] Fix the bug in codegen for ordered directive.
Mon, Sep 6, 6:27 AM · Restricted Project, Restricted Project
peixin added inline comments to D108904: [flang][OpenMP] Added semantic checks for sections (associated section(s) should be structured block(s)) and simd constructs (associated loop(s) should be structured block(s)).
Mon, Sep 6, 4:01 AM · Restricted Project, Restricted Project, Restricted Project

Fri, Sep 3

peixin updated the diff for D108512: [flang][OpenMP] Add semantic checks for ordered construct.

@kiranchandramohan Thanks for your review. Delete SOURCE and SINK in dependence type in parser.

Fri, Sep 3, 9:25 AM · Restricted Project
peixin added a comment to D108904: [flang][OpenMP] Added semantic checks for sections (associated section(s) should be structured block(s)) and simd constructs (associated loop(s) should be structured block(s)).

According to the spec document, I had to deal with orphaned section (without an enclosing sections). I added a test case for the same. However, presently, the parser itself is dealing correctly with the issue by giving errors with very less contextual information. Somewhere down the line, we may need to shift this check from the Parser to Semantics, or improve error reporting in the Parser itself. For now, the test case is a XFAIL with a TODO added to it.

I understand this. What I mean is your implementation is not consistent with the summary, which should be fixed since this patch has not implement the semantic check with informative error message. Otherwise, the summary, which should be the commit message, is confusing.

Fri, Sep 3, 9:04 AM · Restricted Project, Restricted Project, Restricted Project
peixin added a comment to D108904: [flang][OpenMP] Added semantic checks for sections (associated section(s) should be structured block(s)) and simd constructs (associated loop(s) should be structured block(s)).

I think your implementation does deal with the first point in your summary. You only add one test case, right?

Fri, Sep 3, 12:53 AM · Restricted Project, Restricted Project, Restricted Project
peixin updated the diff for D108512: [flang][OpenMP] Add semantic checks for ordered construct.

Replace GetContext.clauseSource() with itr->second->source since GetContext.clauseSource() points the last clause in the clauselist while itr->second->source points the clause being checked. This fix makes the informative error pointing to the right code.

Fri, Sep 3, 12:24 AM · Restricted Project

Thu, Sep 2

peixin committed rGa42380ce8379: [OMPIRBuilder] Add ordered directive to OMPBuilder (authored by peixin).
[OMPIRBuilder] Add ordered directive to OMPBuilder
Thu, Sep 2, 6:40 PM
peixin closed D107430: [OMPIRBuilder] Add ordered directive to OMPIRBuilder.
Thu, Sep 2, 6:40 PM · Restricted Project, Restricted Project
peixin added a comment to D107430: [OMPIRBuilder] Add ordered directive to OMPIRBuilder.

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.

Thu, Sep 2, 8:56 AM · Restricted Project, Restricted Project
peixin added a comment to D107430: [OMPIRBuilder] Add ordered directive to OMPIRBuilder.

@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 now or does it need more review?

Thu, Sep 2, 8:53 AM · Restricted Project, Restricted Project
peixin abandoned D108530: [flang][OpenMP] Fix one bug in parsing depend(sink: vec).
Thu, Sep 2, 5:55 AM · Restricted Project

Thu, Aug 26

peixin updated the diff for D108512: [flang][OpenMP] Add semantic checks for ordered construct.

This patch is based on D94087.
@sameeranjoshi I did some changes from your implementation. Please let me know if you think it is not OK.

Thu, Aug 26, 4:23 AM · Restricted Project

Aug 24 2021

peixin added a comment to D108530: [flang][OpenMP] Fix one bug in parsing depend(sink: vec).

I think resolving the name in flang/lib/Semantics/resolve-directives.cpp is better since some semantic checks can be done at the same time. I will use the method in D94087. This PR may can be closed.

Aug 24 2021, 8:01 AM · Restricted Project
peixin added a comment to D108530: [flang][OpenMP] Fix one bug in parsing depend(sink: vec).

@kiranchandramohan Thanks for pointing out the problem. I think you are right.
I guess the problem is that the name resolution has not been finished. I tried the following change and the command ninja check-flang works fine.

Aug 24 2021, 6:35 AM · Restricted Project

Aug 23 2021

peixin added a comment to D108512: [flang][OpenMP] Add semantic checks for ordered construct.

@kiranchandramohan @kiranktp Thanks a lot. I will proceed with this patch to take it to completion and add @sameeranjoshi as a co-author.
In addition, do I need to add D94087 as a reference in commit message?

Aug 23 2021, 3:54 AM · Restricted Project

Aug 22 2021

peixin added a comment to D94087: [flang][openmp]At most one threads, simd and depend clause can appear on OpenMP ORDERED construct..

BTW, the error of no symbol found in depend(sink: vec) should be fixed by https://reviews.llvm.org/D108530.

Aug 22 2021, 7:36 PM · Restricted Project, Restricted Project
peixin requested review of D108530: [flang][OpenMP] Fix one bug in parsing depend(sink: vec).
Aug 22 2021, 7:16 PM · Restricted Project

Aug 21 2021

peixin requested review of D108512: [flang][OpenMP] Add semantic checks for ordered construct.
Aug 21 2021, 9:53 AM · Restricted Project

Aug 19 2021

peixin updated the diff for D107430: [OMPIRBuilder] Add ordered directive to OMPIRBuilder.

@Meinersbur Thanks for your reply. Add emitting the terminator and finalize and verfyModule calls.

Aug 19 2021, 12:41 AM · Restricted Project, Restricted Project

Aug 17 2021

peixin committed rG3883e266f4ab: [flang][OpenMP] Add semantic check for target nesting (authored by peixin).
[flang][OpenMP] Add semantic check for target nesting
Aug 17 2021, 6:46 PM
peixin closed D106165: [flang][OpenMP] Add semantic check for target nesting.
Aug 17 2021, 6:46 PM · Restricted Project
peixin added a comment to D107430: [OMPIRBuilder] Add ordered directive to OMPIRBuilder.

@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 update_cc_test_checks.py and commits.

Thanks for the notice. I followed the other OMPIRBuilder PRs in order to make review work easily. It should do not matter my manual changes are dropped the next time.

The code seems derived from @fghanim single/master/etc implementation, would be nice if they could have a look.

Yes. He is in the reviewers list.

The non-OMPBuilder code emits an outlined __captured_stmt, but not with OMPBuilder enabled. I assume this is due to the missing finatlize call.

Sorry about the misguide. It is not due to missing finalize call. My last version of patch only implements the code for ordered threads. So I use it for ordered simd test. The non-OMPIRBuilder code emits the outlined function call for ordered simd, while emits the inlined statements for ordered threads. Now the non-OMPIRBuilder code and OMPIRBuilder code generate the similar IRs.

Aug 17 2021, 6:46 AM · Restricted Project, Restricted Project
peixin updated the diff for D107430: [OMPIRBuilder] Add ordered directive to OMPIRBuilder.

Support ordered directive in OMPIRBuilder to OMP 5.0 standard. That is, support ordered directive without clause, with threads, simd or depend clause.

Aug 17 2021, 6:27 AM · Restricted Project, Restricted Project
peixin committed rG6d952b08bdac: [NFC] Fix typos (authored by peixin).
[NFC] Fix typos
Aug 17 2021, 2:21 AM

Aug 14 2021

peixin updated the diff for D106165: [flang][OpenMP] Add semantic check for target nesting.

Fix the clang format warning.

Aug 14 2021, 4:47 AM · Restricted Project

Aug 13 2021

peixin updated the diff for D106165: [flang][OpenMP] Add semantic check for target nesting.

Rebase this patch.

Aug 13 2021, 11:30 PM · Restricted Project
peixin added a comment to D106538: [flang][OpenMP] Add semantic check for cancellation nesting.

@Leporacanthicus @kiranchandramohan Thanks for the help.

Aug 13 2021, 10:22 AM · Restricted Project

Aug 9 2021

peixin updated the diff for D107430: [OMPIRBuilder] Add ordered directive to OMPIRBuilder.

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.

Aug 9 2021, 5:59 AM · Restricted Project, Restricted Project
peixin updated the diff for D106538: [flang][OpenMP] Add semantic check for cancellation nesting.

@kiranchandramohan Thanks for the review. Added the comments.

Aug 9 2021, 5:33 AM · Restricted Project

Aug 5 2021

peixin updated the diff for D107430: [OMPIRBuilder] Add ordered directive to OMPIRBuilder.

@Meinersbur Thanks for the review.

Aug 5 2021, 2:22 AM · Restricted Project, Restricted Project
peixin updated the diff for D106165: [flang][OpenMP] Add semantic check for target nesting.

Fix the clang-format warning.

Aug 5 2021, 12:06 AM · Restricted Project

Aug 4 2021

peixin added a comment to D107430: [OMPIRBuilder] Add ordered directive to OMPIRBuilder.

@kiranchandramohan Thanks for the review. Removed the OpenMP 1.0 target from the description.

Aug 4 2021, 6:15 AM · Restricted Project, Restricted Project
peixin updated the summary of D107430: [OMPIRBuilder] Add ordered directive to OMPIRBuilder.
Aug 4 2021, 5:45 AM · Restricted Project, Restricted Project
peixin requested review of D107430: [OMPIRBuilder] Add ordered directive to OMPIRBuilder.
Aug 4 2021, 12:54 AM · Restricted Project, Restricted Project

Jul 31 2021

peixin updated the diff for D106165: [flang][OpenMP] Add semantic check for target nesting.

@kiranchandramohan Thanks for the review.
Replaced the variable name directive with ineligibleTargetDir.

Jul 31 2021, 7:52 PM · Restricted Project

Jul 30 2021

peixin added a comment to D106165: [flang][OpenMP] Add semantic check for target nesting.

@kiranchandramohan Thanks for the review.

Jul 30 2021, 8:18 PM · Restricted Project

Jul 29 2021

peixin added a comment to D106165: [flang][OpenMP] Add semantic check for target nesting.

@kiranchandramohan Thanks for the comments.

Jul 29 2021, 5:19 AM · Restricted Project
peixin updated the diff for D106165: [flang][OpenMP] Add semantic check for target nesting.

I will rebase this patch or https://reviews.llvm.org/D106335 after either of them is merged.

Jul 29 2021, 5:16 AM · Restricted Project
peixin updated the diff for D106538: [flang][OpenMP] Add semantic check for cancellation nesting.

Add the following test scenarios, which is taskgroup nested between parallel and cancel taskgroup and fix the implementation by break the for loop when encountering taskgroup or parallel/target parallel.

Jul 29 2021, 4:48 AM · Restricted Project
peixin added a comment to D106538: [flang][OpenMP] Add semantic check for cancellation nesting.

Found two more scenarios that are not covered. Please wait for the new patch to review in a few minutes.

Jul 29 2021, 4:24 AM · Restricted Project
peixin updated the diff for D106335: [flang][OpenMP] Add semantic check for teams nesting.

@kiranchandramohan Thanks for the suggestions, which prevents from using magic number. Replaced directiveNest_[3] with directiveNest_[LastType+1].

Jul 29 2021, 4:22 AM · Restricted Project
peixin added a comment to D106538: [flang][OpenMP] Add semantic check for cancellation nesting.

@kiranchandramohan Thanks for the explanation and review.

Jul 29 2021, 1:46 AM · Restricted Project
peixin updated the diff for D106538: [flang][OpenMP] Add semantic check for cancellation nesting.
Jul 29 2021, 1:39 AM · Restricted Project
peixin updated the diff for D106335: [flang][OpenMP] Add semantic check for teams nesting.

Change directiveNest_[2] to directiveNest_[3] since one more element LastType was added.

Jul 29 2021, 1:24 AM · Restricted Project
peixin added a comment to D106335: [flang][OpenMP] Add semantic check for teams nesting.

@kiranchandramohan Thanks for the review. Fixed the patch according to your comments.

Jul 29 2021, 12:34 AM · Restricted Project
peixin updated the diff for D106335: [flang][OpenMP] Add semantic check for teams nesting.
Jul 29 2021, 12:27 AM · Restricted Project

Jul 22 2021

peixin added a comment to D106538: [flang][OpenMP] Add semantic check for cancellation nesting.

Currently, I do not implement the semantic check for the following two scenarios, which should not be allowed according to OpenMP Version 5.0 Specification.

!$omp task
  !$omp cancel taskgroup
!$omp end task
!$omp taskloop nogroup
  do i = 1, N
    !$omp cancel taskgroup
    a = 3.14;
  end do

They are supported by gfortran and current clang. Should we report errors or warnings or just ignore it? Or maybe my understanding is incorrect.

Jul 22 2021, 4:20 AM · Restricted Project
peixin requested review of D106538: [flang][OpenMP] Add semantic check for cancellation nesting.
Jul 22 2021, 4:18 AM · Restricted Project

Jul 21 2021

peixin added inline comments to D106165: [flang][OpenMP] Add semantic check for target nesting.
Jul 21 2021, 1:29 AM · Restricted Project

Jul 20 2021

peixin added inline comments to D106335: [flang][OpenMP] Add semantic check for teams nesting.
Jul 20 2021, 11:24 PM · Restricted Project
peixin updated the diff for D106335: [flang][OpenMP] Add semantic check for teams nesting.

@clementval Thanks for the review.

Jul 20 2021, 8:04 AM · Restricted Project
peixin updated the diff for D106165: [flang][OpenMP] Add semantic check for target nesting.

@kiranchandramohan Thanks for the review.

Jul 20 2021, 12:55 AM · Restricted Project
peixin updated the summary of D106335: [flang][OpenMP] Add semantic check for teams nesting.
Jul 20 2021, 12:18 AM · Restricted Project

Jul 19 2021

peixin added reviewers for D106335: [flang][OpenMP] Add semantic check for teams nesting: kiranchandramohan, clementval, richard.barton.arm, sameeranjoshi, praveen, SouraVX, arnamoy10, bryanpkc.
Jul 19 2021, 11:13 PM · Restricted Project
peixin updated the diff for D106335: [flang][OpenMP] Add semantic check for teams nesting.
Jul 19 2021, 9:01 PM · Restricted Project
peixin updated the diff for D106335: [flang][OpenMP] Add semantic check for teams nesting.

Fix errors info in related test cases.

Jul 19 2021, 9:00 PM · Restricted Project
peixin requested review of D106335: [flang][OpenMP] Add semantic check for teams nesting.
Jul 19 2021, 7:52 PM · Restricted Project
peixin updated the diff for D106165: [flang][OpenMP] Add semantic check for target nesting.

Set the flag eligibleTARGET as true by default and print the error when it is false.

Jul 19 2021, 12:11 AM · Restricted Project

Jul 16 2021

peixin updated the diff for D106165: [flang][OpenMP] Add semantic check for target nesting.

@clementval Thanks for the comments. Upload the following update:

  1. I am reminded that the counter is not necessary since the if branch dirContext_.size() >= 1 already checks the directive nested. Delete the counter usage and checking GetContext().directive == llvm::omp::Directive::OMPD_target inside dirContext_.size() >= 1 check is enough.
  2. Add the error check in test case.
  3. There is no this error info before, or at lease I did not find in f18 openmp semantic check. I test gfortran 9.3.0 and it gives the following warning:
Jul 16 2021, 7:17 PM · Restricted Project
peixin requested review of D106165: [flang][OpenMP] Add semantic check for target nesting.
Jul 16 2021, 10:19 AM · Restricted Project

Jul 14 2021

peixin added a comment to D105874: [flang][OpenMP] Fix semantic check of test case in taskloop simd construct.

Thanks for the help. @kiranchandramohan @arnamoy10

Jul 14 2021, 6:42 AM · Restricted Project

Jul 13 2021

peixin added a comment to D105874: [flang][OpenMP] Fix semantic check of test case in taskloop simd construct.

Thanks for the comments. Update the following:

  1. Remove the semantic check of taskloop simd reduction in check-omp-structrue.cpp.
  2. Fix the test case of omp-taskloop-simd01.f90 according to OpenMP 5.0 specification.
  3. Fix the title and summary.
Jul 13 2021, 6:39 PM · Restricted Project
peixin updated the diff for D105874: [flang][OpenMP] Fix semantic check of test case in taskloop simd construct.
Jul 13 2021, 6:34 PM · Restricted Project
peixin added a comment to D105874: [flang][OpenMP] Fix semantic check of test case in taskloop simd construct.

@clementval Yes, I agree. I did not notice that this restriction was deleted in Openmp-5.0 standard when I upload the first version of this patch. Do you think if I should fix the restriction info in omp-taskloop-simd01.f90 and the typos in omp-clause-validity01.f90, and change the commit message by stating that this restriction is deleted in openmp version 5.0?

Jul 13 2021, 8:43 AM · Restricted Project
peixin added a comment to D105874: [flang][OpenMP] Fix semantic check of test case in taskloop simd construct.

@kiranchandramohan Thanks for the guide to submit patches and the comments.

Jul 13 2021, 6:46 AM · Restricted Project
peixin updated the diff for D105874: [flang][OpenMP] Fix semantic check of test case in taskloop simd construct.
Jul 13 2021, 6:39 AM · Restricted Project
peixin updated the diff for D105874: [flang][OpenMP] Fix semantic check of test case in taskloop simd construct.
Jul 13 2021, 6:26 AM · Restricted Project
peixin added a reviewer for D105874: [flang][OpenMP] Fix semantic check of test case in taskloop simd construct: bryanpkc.
Jul 13 2021, 6:26 AM · Restricted Project
peixin updated the diff for D105874: [flang][OpenMP] Fix semantic check of test case in taskloop simd construct.

Add ! REQUIRES: shell in omp-taskloop-simd01.f90.

Jul 13 2021, 12:55 AM · Restricted Project

Jul 12 2021

peixin requested review of D105874: [flang][OpenMP] Fix semantic check of test case in taskloop simd construct.
Jul 12 2021, 11:34 PM · Restricted Project