[PATCH] D89671: [LLVM][OpenMP] Adding support for OpenMP sections construct in OpenMPIRBuilder

[Fady Ghanim via Phabricator via llvm-commits]

fghanim added inline comments.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:912
+         "Unexpected Directive for Finalization call!");
+  Fi.FiniCB(InsertPointTy(SwitchExitBB, SwitchExitBB->getFirstInsertionPt()));
+
----------------
AMDChirag wrote:
> fghanim wrote:
> > What I meant by my comment; is that you will use the same finiCB multiple times - once per section. you should undo the last changes, However, no need to do add anything back, but based on your clang impl. this is not the place for it.
> > every `sections` directive has one or more `section` directive, and based on clang impl. that thing is generated separately.  The finalization block will be generated inside the `section` directive.
> > 
> > Implementation-wise; this translates to writing a `createSection` which can be implemented as an inlined region, look at `createCritical` for an example of how to do it. The clang side is also quite easy - again look at `emitOMPCritical` in clang for an example.
> > 
> > I am **not ** going to require that you write `createSection` along with its clang usage to accept this patch, **unless ** the clang lit test for `omp sections` doesn't work without it.
> So, to confirm,
> There should be another function called `CreateSection` which should be called from within `CreateSections` for each of the `section`s, and that would use the FiniCB callback?
> Something like
> ```
> CreateSections() {
> ...Init...
> for each section {
>     call CreateSection(SectionCB, FiniCB)
> }
> ```
> Where CreateSection will use the `EmitOMPInlinedRegion()` function?
> 
> Also, the clang lit test case for the clang side of things will be in the clang patch. I'll be creating the test.
Basically yes. to be extra specific :D
The way clang and the OMPBuilder will end up handling this
```
clang.emitparallel() 
  -->OMPBuilder.creatparallel( ..., bodyCB ,...) 
    --> BodyCB ()
      --> clang.emitSections() 
        --> OMPBuilder.createSections( ..., SectionCB[N], ...) 
          --> SectionCB[i]()
            --> clang.emitSection() 
              --> OMPBuilder.createsection()
                 --> {this will contain the body of section[i] and finalization code for it - essentially the same finiCB used once per section).
```


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:1152-1155
+  EXPECT_EQ(F, OutlinedFn);
+  EXPECT_FALSE(verifyModule(*M, &errs()));
+  EXPECT_EQ(OutlinedFn->arg_size(), 1U);
+  EXPECT_EQ(OutlinedFn->getBasicBlockList().size(), 8U);
----------------
fghanim wrote:
> I don't think these checks are adequate. Generally, what you should be testing for is the generator's "defining features" - for lack of better name. examples for things you should be testing
> - your loop - do we have the proper bounds, does it have the correct checks, is it generating the correct branches to name a few. 
> - are we generating correct sections, and are they terminated appropriately (i.e. do they branch to correct/corresponding FiniCB) etc.
> - is the `switchInst` being created correct (correct number of cases, branches to correct blocks for each case, etc.)
> - all omp runtime calls (are we generating all correct runtime calls, correct arg., correct locations, etc.)
> - any other thing you think is important and needs to be regularly tested
Don't forget about this. please :)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89671/new/

https://reviews.llvm.org/D89671