- User Since
- May 5 2017, 1:55 PM (205 w, 3 d)
Sun, Apr 11
Sat, Apr 10
Fri, Apr 9
Two first comments. I will do a detailed review later.
Thu, Apr 8
LGTM. Thanks for the changes and the tests.
Wed, Apr 7
Might be good to have a test for fdebug-dump-parse-tree-no-sema.
Tue, Apr 6
@PeteSteinfeld I have reverted this patch since I could not find a quick fix.
We have a buildbot CI failure due to this patch.
Mon, Apr 5
Fri, Apr 2
@klausler We have a buildbot failing with the following error. https://lab.llvm.org/buildbot/#/builders/33/builds/3439
Tue, Mar 30
Thu, Mar 25
I made a trivial fix to get the builds to pass.
Tue, Mar 23
LGTM. Thanks Peter for fixing this quickly.
Mon, Mar 22
Sat, Mar 20
The OpenMP for Flang team has consistently been asking for upstreaming the fir-dev branch. I made an initial attempt to upstream a portion in May last year (https://reviews.llvm.org/D79731) which was discarded since it
did not have any community support. The current situation presents a few difficulties for the OpenMP team.
- The OpenMP dialect is in the llvm-project repo, while FIR codegen is developed in the fir-dev repo. So we have to first make a patch to llvm-project/mlir and then wait for it to be merged into the fir-dev repo (which can take from one to two weeks) and then make the relevant changes in the fir-dev repo. If FIR codegen was also upstream then this delay and committing to multiple repositories can be avoided.
- Since the bridge code (parse-tree to FIR) and codegen is not available in llvm-project/flang, any commits that we make to fir-dev cannot be upstreamed. So all our changes are also increasing the diff between fir-dev and upstream llvm-project/flang. Left uncontrolled this might become an untameable monster and we might never be able to fully upstream fir-dev.
- Since the OpenMP code which works with FIR is in fir-dev we cannot often show the context to MLIR core team. On at least one occasion this has become an issue while seeking help. If the code is upstream this will facilitate better discussions with the MLIR core team.
Thu, Mar 18
Failing a buildbot https://lab.llvm.org/buildbot/#/builders/33/builds/3150.
Addressed review comments.
Wed, Mar 17
Tue, Mar 16
Mon, Mar 15
Thanks for this patch. Couple of nits.
Just a quick comment. I will review in detail later.
Mar 13 2021
Address review comments.
Updated syntax to specify the type along with the index.
Using BNF for parsing function documentation
Minor fixes, using llvm::interleaveComma.
@arnamoy10 can you submit this?
LGTM. Approving quickly since it fixes failures. Might need to do a recce and check that usage is consistent.
Mar 11 2021
Added multi-block tests.
Mar 10 2021
Fixed failure in SCFToOpenMP.
Converted "->" to "=" for linear clause.
Ping @llitchev. Would you have time to take this forward?
Mar 9 2021
Mar 7 2021
LGTM. Yes, using the symbol is the right way here.
Thanks @yhegde for separating this out into a new patch. Thanks also for you patience and all your work for semantic checks for OpenMP.
LGTM. Thanks for the patience.
Mar 6 2021
Mar 5 2021
If popping the end_sections helps you move forward, I am fine with it. Please go ahead.
Mar 4 2021
Thanks @arnamoy10 for this patch.
Mar 3 2021
Mar 2 2021
I see a failure in the buildbot.
Mar 1 2021
The aarch64 flang buildbots are failing since this commit.
The x86 buildbot is passing. https://lab.llvm.org/buildbot/#/builders/132
Feb 28 2021
There are a few comments not marked as done. Are these not done or have been just not marked done?
If you can add the autogen portion that will be great. It can be added either as part of this patch or a separate patch. But it need not be a blocker for this. I am approving this patch.
Feb 26 2021
Did you miss the fir-ops tests for the new ops?
LGTM. This will be tested in the subsequent usage.
Feb 24 2021
Feb 23 2021
A few more questions.
Quick comments/questions before I have a final look.
Can you mark all comments which you believe are handled as done?
Feb 22 2021
A few nits, comments and clarifications.
Feb 17 2021
I guess for small format/clang-tidy changes reviews are not required.
Feb 14 2021
LGTM. I have a Nit comment.
Feb 11 2021
One question about blocks and a few nits.
Feb 10 2021
A few questions and change requests.
Thank you for the review comments @kiranchandramohan . Can I split this patch one with threadprivate and cycle , dowhile etc with another patch ?
Feb 9 2021
LGTM. Please address the clang-format comment before submission.
Thanks @yhegde for the changes. I believe this threadprivate check needs some more design and work. I don' t think we can clear the threadprivate symbols in module subprograms because it will fail to catch the error in test1 below. I think the threadprivate information should be captured in a symbol so that it is available in places where the module is used like in test2.
Feb 8 2021
LGTM. Please consider Mehdi's comments before submitting.