Thanks @kiranchandramohan for helping out in this :). I'm abandoning this in favor of your approach. (Updating the PR also)
Thanks @tskeith for suggestion, I'll take a note of this.

Thanks @kiranchandramohan for suggestion. I already tried this thing(didn't worked out).
Unless I'm missing something: There's a type mismatch while interfacing this function. Here's the snippet of usability

case Fortran::parser::OmpProcBindClause::Type::Master:
          parallelOp.proc_bind_valAttr(firOpBuilder.getStringAttr(
           omp::stringifyClauseProcBindKind(procBindClause->v)));
          break;

Error:

error: cannot convert ‘const Fortran::parser::OmpProcBindClause::Type’ to ‘mlir::omp::ClauseProcBindKind’ for argument ‘1’ to ‘llvm::StringRef mlir::omp::stringifyClauseProcBindKind(mlir::omp::ClauseProcBindKind)’
              omp::stringifyClauseProcBindKind(procBindClause->v)));

An alternative solution would be: (but it's hard coding) ? (do you vote for this?)

case Fortran::parser::OmpProcBindClause::Type::Master:
          parallelOp.proc_bind_valAttr(firOpBuilder.getStringAttr(
           StringRef("master")));
          break;

Ping! @ all reviewers.

In D89340#2331031, @kiranchandramohan wrote:

I guess @clementval or @rriddle can approve the new extra functions added based on its need in Flang parse-tree lowering or on similar usage for StrEnumAttributes in rest of MLIR.

Snippet of usability from an upcoming/in-progress patch.

 switch (procBindClause->v) {
+                     case Fortran::parser::OmpProcBindClause::Type::Master:
+                     parallelOp.proc_bind_valAttr(firOpBuilder.getStringAttr(parallelOp.getProcBindValueMasterAttrName()));
+                             break;
+                     case Fortran::parser::OmpProcBindClause::Type::Close:
+                     parallelOp.proc_bind_valAttr(firOpBuilder.getStringAttr(parallelOp.getProcBindValueCloseAttrName()));
+                             break;
+                     case Fortran::parser::OmpProcBindClause::Type::Spread:
+                     parallelOp.proc_bind_valAttr(firOpBuilder.getStringAttr(parallelOp.getProcBindValueSpreadAttrName()));
+                             break;

Aside from this, Should we promote/remodel these attributes as UnitAttr? What do you guys think?

I feel the current modelling as an Optional StrEnumAttribute is fine here. proc_bind and default takes a set of values which are best captured by this modelling. If we have these as unit attributes then all the values can occur at the same time unless disabled using the verifier.

Ah, potential oversight of details(Sorry for that!). Anyway I've rolled back to initial version and introduced some extra declaration that can be used by Flang(once this patch is in) while lowering.
Aside from this, Should we promote/remodel these attributes as UnitAttr? What do you guys think?

In D89340#2328518, @rriddle wrote:

What kind of issues? This seems like a regression in terms of usability. Can you describe in detail the problems that you were having?

If we agreed upon this, the next would be to remodel default clause as UnitAttr also.

Thanks for the patch! Overall it's in pretty nice state and conformant to design highlighted.
Could you please clang-format this patch, There are lot of lint warning that causes lot of noise while reviewing.
Couple of NIT comments inline. I've tried marking all but, Could you please finish comments with period.

Thanks for the patch. You may want to wait for @jdoerfert for final sign off!

Thanks for the patch!
NIT:
A more reasonable commit message could be:
[NFCI][flang] Introduce new helper TODO for further development
Or something similar(aligned to intent) :)

In D88706#2309408, @fghanim wrote:

In D88720#2307968, @kiranchandramohan wrote:

@SouraVX In this patch I have removed the OpenMPIRBuilder alloca changes. This contains only the changes to in ModuleTranslation and it seems to work fine. Can you check at your end as well as with the master Operation?

Since it's already approved, feel free to remove test case modification and commit no need for another revision :)

Are you alluding, that this review should be renamed as some "Upstream XXXX code from PR" and nothing else ?

In D88221#2292669, @jdoerfert wrote:

This is not "codegen support" is it? That sounds like "we now support codegen of X". Maybe: "Fix XXX codegen insertion point placement"?

Please use a proper commit message and subject. Linking some other discussion is fine but it should be self contained.

Sorry, I should've done better. Now modified to convey the intent.

Removed unit-test in be1197c403b22291e35cbc5e96788860ceabd40c

Reverse Ping!

• Addressed comments by @jeanPerier and @clementval.
• Rebased to the tip.

Thanks @jeanPerier for reviewing this!

LGTM, thanks!

Thanks @kiranchandramohan for pushing this separately.
LGTM :)

Revereted!
Thanks!

In D87389#2265571, @clementval wrote:

Yes those are duplicate in a sense, but they are still tests(since nothing else is there) and can self validate well-formedness of the operation(which is one of the major pre-requisite for proceeding further with Lowering).
+ They will ensure matches with community criteria. up-streaming in testable chunks.

My principal concern is that they do not test the code in genACC or genOMP so they won't catch any problem or regression there.

I think that is justified and already tested with full coverage in regression(end-to-end) test with bbc.

I'm trying to put in place a unit test that make use of the genACC function directly. If that works I'll add the test before landing the patch.

+1
The reason why I'm saying this that we(you) also don't want both branches going out-of-sync and regressing further development :)

In D87389#2263915, @clementval wrote:

In D87389#2263893, @SouraVX wrote:

Since bbc based test can't be added here, would you mind adding a unit-test here ?

Well I have to see how to add a meaningful unit test here. Lowering unit tests that are currently upstreamed seem to just copy-paste the lowering code which does not really test the lowering code itself.

Since bbc based test can't be added here, would you mind adding a unit-test here ?

Thanks @kiranktp ,@kiranchandramohan for reviewing this!
When adding a test case for a MasterOp inside the region of ParallelOp(typical use case.)
Test snippet:

llvm.func @test_omp_master() -> () {
  omp.parallel {
  "omp.master" ()({
    omp.terminator
  }):()->()
    omp.terminator
}
  llvm.return
}

This produces following crash/assertion failure in OMPIRBuilder:

mlir-translate: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:717: llvm::IRBuilderBase::InsertPoint llvm::OpenMPIRBuilder::CreateParallel(const llvm::OpenMPIRBuilder::LocationDescription&, llvm::OpenMPIRBuilder::InsertPointTy, llvm::OpenMPIRBuilder::BodyGenCallbackTy, llvm::OpenMPIRBuilder::PrivatizeCallbackTy, llvm::OpenMPIRBuilder::FinalizeCallbackTy, llvm::Value*, llvm::Value*, llvm::omp::ProcBindKind, bool): Assertion `Outputs.empty() && "OpenMP outlining should not produce live-out values!"' failed.

digging in further, I realized this Outputs is getting populated with(below instruction) and finally asserting at Outputs.empty().

%omp_global_thread_num2 = call i32 @__kmpc_global_thread_num(%struct.ident_t* @3), !dbg !9

I'll dig further, BTW if you guys have any inputs WRT this. Please share!

I don't know what's the criteria for arc land for removing strings from commit message. I added 2 Co-author: Eric Schweitz and Bhuvanendra kumar(from AMD). But apparently it dropped it when landing the actual patch. :(
Are you guys Okay with this ?
Anyway, I'll modify the summary here as well(for attributing co-author's).

Added comments for reserved bit codes.

In D86305#2230794, @SouraVX wrote:

In D86305#2230724, @aprantl wrote:

I'm fine with whatever numbering makes your development the easiest. My only ask is that if we still have a gap after this, please document what nodes are expected to go there, so there 1) won't be any future conflicts and 2) we don't pointlessly reserve space.

I'll have to re-confirm with our team. Making sure we don't break anything is priority for us.
@schweitz do you have any reservations WRT this ?

In D86305#2230724, @aprantl wrote:

I'm fine with whatever numbering makes your development the easiest. My only ask is that if we still have a gap after this, please document what nodes are expected to go there, so there 1) won't be any future conflicts and 2) we don't pointlessly reserve space.

In D86305#2230711, @schweitz wrote:

You might want to add "[flang]" to the subject line as this change supports Fortran. Thanx.

Updated BitCodes for DIString to 41, this will narrow down the gap. Moving forward when introducing new metadata we can use 42-43 bits to compensate the gap.

Thanks @aprantl for reviewing this. Let's wait for @schweitz also to have a look. Planning to land this tomorrow or so.

Addressed @aprantl review comments. Thanks for reviewing this!
Changes:

• Reduced to minimal variants.
• And other Nit comments.

Thanks @bjope for catching this bug! I do not have big-endian machine so it must've got un-noticed(test case for 4 bytes float was written and tested/verified on x86 machine).
But I got your point, I'll see if I can add a hand written test case to address this situation.