- User Since
- Oct 25 2019, 12:31 PM (65 w, 3 d)
Sat, Jan 16
Thu, Jan 7
Ping. Please add the Lit test for this.
Dec 6 2020
Thanks for the patch. Just a couple of nits
Dec 1 2020
Thanks; just two more mediocre things if possible. if not. you are good to go. :)
Nov 30 2020
Thank you for the patch :)
Nov 26 2020
Nov 20 2020
Nov 18 2020
Nov 10 2020
I second @Meinersbur comment. Reducing duplication is always better :)
Thanks for doing this.
Nov 9 2020
Nov 7 2020
Thanks for the response, I think I am beginning to understand where this fits.
Just one Nit, O/W LGTM
Nov 5 2020
Thanks for the Patch. Generally looks Good.
Just a couple of very minor comments/questions
Oct 21 2020
Great work. Thanks.
Oct 20 2020
Oct 19 2020
Thank you for working on this!
Oct 2 2020
While debugging the nested parallel region issues, we saw some difference between where the allocas are placed by the OpenMP IRBuilder in the clang usage and the MLIR usage. Moving to the version where the OpenMP IRBuilder maintains allocaIP fixed the difference. In MLIR module translation there is no alloca insertion point. So what we can provide as allocaIP is the current insertionPoint.
Oct 1 2020
After reading the patch for master you referred to, I don't understand why do we need the OMPBuilder to maintain the insertion point. As far as master is concerned, we will emit any alloca's contained inside its region into the entry block of the enclosing outlined region (e.g. innermost parallel).
FWIW, the master directive in clang already uses the OMPBuilder and just relies on clang to handle the insertion of any non-omp code (including alloca's). Is there a reason why a similar approach wouldn't work here?
Sep 18 2020
I have no idea if this has been considered before or not, or if there is some technical/organizational difficulties against this option, but I think an option would be to create somewhere in the LLVM umbrella project for common Headers/Def.s between different LLVM subprojects (directory called "common" maybe?), which should help maintain independence between the projects without duplication, package maintainers will know to always include that in any package without having to worry about looking for shared files on a file by file bases, and users should always download that with whichever llvm package they are going to use.
Sep 8 2020
@kiranchandramohan This error happens when there are things within the OMP region still alive/are used outside the region, I cannot comment on what causes this here. Usually, this suggests that "something" that should be contained within the parallel region is not being detected as such, and so when the region is outlined, this User remains as part of the original function.
I don't know enough about the OMPIR for me to be helpful, however, I noticed a couple of things that I wanted to ask about for better understanding. :)
Sep 1 2020
Aug 31 2020
Aug 15 2020
Aug 14 2020
Aug 13 2020
Aug 12 2020
Aug 10 2020
Thanks. will fix nits.
Aug 9 2020
Feel free to add other reviewers. Thanks.
Feel free to add other reviewers. Thanks.
Jul 17 2020
Great. Thank you!
Jul 16 2020
Jul 14 2020
I'll address the nits.
Jul 13 2020
Thanks for the update. Just a couple of Nits, and a quick note
Jul 3 2020
Jul 2 2020
Jun 30 2020
Jun 29 2020
OK. Thanks :)
Jun 28 2020
Thanks for working on this. LGTM.
Did you make any changes other than splitting from D82470 ?
Jun 26 2020
Build error due to unused variable fixed in rG89812eeee97c8f7ab2e6ee2c48edb7a409dfff39
Jun 24 2020
Thanks for the Patch. I have few questions to help me understand what's going on.
Jun 15 2020
ping - please suggest reviewers I can add to review the clang side of things?
Does this patch need further changes?
Jun 10 2020
you are responding to a comment from 2 weeks ago, so let's just move on.
Jun 9 2020
@jdoerfert Please suggest reviewer's for this, and I will add them to other clang related patches
- addressing reviewer's comments
- Rebase + refactor based on D80222
- addressed reviewer comments
May 27 2020
I am moving on because we are not getting anywhere. However, There are few things I need to point out very quickly.
I fail to see the point in committing for example your target type solution if we found a more generic version in the meantime.
We can for sure commit them and then replace them subsequently, but is that really helping anyone? It would not be a question if
they were in, since they are not it seems to me there is no benefit in blocking the other patch on them. I mean, the time you worked
on that part is not "less wasted" if we commit it. TBH, I don't thin it is wasted at all but that is a different conversation.
May 22 2020
I am going to omit parts of the quote, because who wants to look at a wall of test - readability is important ;)
May 21 2020
May 20 2020
Addressing more reviewers comments.
"Soon" is indeed relative, and so is "later", and so is 99% of the words. However, words have specific meanings, otherwise opposites would refer to the same thing, and words become useless and meaningless. "Soon" means soon.
May 19 2020
Addressing reviewer Comments
May 14 2020
updating in response to review comments
- removed many Definitions from OMPKinds.def due to being added in D79739
- made changes based on reviewer comments
- added unit test for CreateCopyinClauseBlocks()
May 13 2020
May 12 2020
This is a small patch as it is. splitting it any further would make it very very small :D
@jdoerfert At this point should I just drop the changes I made in OMPKinds.def ? :D
May 9 2020
Apr 2 2020
OK, As I said a few days ago, I went over the patch, and I didn't see any functional changes. I am accepting this patch.
Mar 31 2020
Figured as much, just wanted to be sure. Anyways, this one also LGTM
I'll wait a couple of days in case any one has comments, if not I'll approve it
Thanks for working on this as well.
As an aside, I like the new allowed clause implementation much better. it is much simpler and cleaner than the previous one.
I'll wait to see if anyone else has comments, but if not, then it LGTM.
Thanks for doing this. I looked at all of it, and the changes seem to be you just moved things to llvm/Frontend, and fixed namespaces/includes to work correctly with the new location. Is there here anything else I am missing?
Mar 20 2020
My comments were nits so.