Thanks for the review :)

• Addressing reviewer's comments

• clang-tidy + clang-format fixes

In D99024#2694893, @tianshilei1992 wrote:

Thanks for the patch! FYI, I'm working on the FE part of compare and compare capture.

• Rebasing
• Adding test cases
• Addressing reviewers comments

This is generally fine with me, @fghanim @Meinersbur any concerns?

You can update the tests as long as long as the output is correct. for example the difference is only in names, ordering of basicblocks and instructions that doesn't affect correctness, etc.

ping

LGTM from me

fixing some clang-tidy issues

• addressing more reviewer comments
• Fixing nits + clang-format errors

In D99024#2667320, @SouraVX wrote:

Thanks! @fghanim for the patch :)

• more clang-tidy fixes

• fixed clang-tidy errors
• addressed reviewer comments

Ping. Please add the Lit test for this.

Thanks for the patch. Just a couple of nits

Thanks; just two more mediocre things if possible. if not. you are good to go. :)

Thank you for the patch :)

I second @Meinersbur comment. Reducing duplication is always better :)

Thanks for doing this.

LGTM

Thanks for the response, I think I am beginning to understand where this fits.
Just one Nit, O/W LGTM

Thanks for the Patch. Generally looks Good.
Just a couple of very minor comments/questions

In D87247#2341467, @SouraVX wrote:

• At https://github.com/llvm/llvm-project/blob/master/llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp#L918 @jdoerfert could you please explain the need for passing empty AllocaIP ? I meant this will be empty? while interfacing MasterOp from flang/mlir. Why is that.

Great work. Thanks.

In D89671#2340761, @AMDChirag wrote:

I have updated the title.

Thank you for working on this!

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.

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?

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.

@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. :)

In D79677#2248663, @lebedev.ri wrote:

Tests missing

In D85619#2248195, @kiranchandramohan wrote:

What is the plan for this patch?

In D79677#2248197, @kiranchandramohan wrote:

What is the plan for this patch?

ping

ping

Fixing nits

Thanks. will fix nits.

In D85619#2208425, @kiranchandramohan wrote:

Before I start, Why is this change all new code and no modification or deletion of existing Clang functionality for omp single/copyprivate?

Feel free to add other reviewers. Thanks.

Feel free to add other reviewers. Thanks.

Great. Thank you!
LGTM

I'll address the nits.

Thanks :)

Thanks for the update. Just a couple of Nits, and a quick note

LGTM

In D79677#2119019, @kiranchandramohan wrote:

Is the ordering of code generation for clauses important?
copyin -> firstprivate -> barrier -> private

OK. Thanks :)

Thanks for working on this. LGTM.
Did you make any changes other than splitting from D82470 ?

Build error due to unused variable fixed in rG89812eeee97c8f7ab2e6ee2c48edb7a409dfff39

commited: rG82b8236cf248

Thanks for the Patch. I have few questions to help me understand what's going on.

ping - please suggest reviewers I can add to review the clang side of things?

Ping.
Does this patch need further changes?

you are responding to a comment from 2 weeks ago, so let's just move on.

• rebase
• splitting patch into 4 ( this, D81482 , D81483 , D81484 )
• addressing reviewer's comments

@jdoerfert Please suggest reviewer's for this, and I will add them to other clang related patches

• rebase
• addressing reviewer's comments

• Rebase + refactor based on D80222
• addressed reviewer comments

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.

At one point, you said I was delaying D80222 moments after it was uploaded. Now, D79675 and D79676 , cannot be committed because of the artificial dependency on that patch.

I am going to omit parts of the quote, because who wants to look at a wall of test - readability is important ;)

Addressing more reviewers comments.