[OpenMP] Fixes for unstructured code in OpenMP regions

[kiranchandramohan]

The following changes are made for OpenMP operations with unstructured regions to ensure that empty region blocks are created only once. This helps fix issues seen in the SNAP application.

1. For combined constructs the outer operation is considered a structured region and the inner one as the unstructured.
2. Added a condition to ensure that we create new blocks only once for nested unstructured OpenMP constructs.

Tests are added for checking the structure of the CFG.

Fixes #1192

[vdonaldson]

Some general comments -

It might be a good idea to split off the cherry pick changes that avoid the buggy error message. That fix is a prerequisite to, and independent of other fixes.

You can get additional testing of unstructured code by forcing unstructured code generation for all loops with the -no-structured-fir developer flag. The intent is to make it easier to exercise less common unstructured code cases without actually having to put in unstructured branches.

[vdonaldson]

Kiran - PFTBuilder.h has a template for recognizing individual statements. Given an eval e, it should be possible to query if e.isA<parser::ContinueStmt>.

However, if the idea is to find the block containing an "exit" ContinueStmt, it should be possible to get that automatically, in which case special case code should not be needed. Like other constructs, a DoConstruct has a constructExit eval that should be set to that eval. Depending on the eval passed to createBodyOfOp that constructExit eval may already be directly available. Given that exit eval, its block member is the block of interest.

[kiranchandramohan]

Thanks, I completely missed that, I have switched to using isAparser::ContinueStmt.

Yes, good point about the constructExit. Question is what about for other non-looping constructs?

Also, going back to a point you raised in #1192, quoting you below.
"although there is some question as to whether the ContinueStmt should be inserted before or after the OmpEndLoopDirective Evaluation. (It may be that either way is ok.)"

I think the reason the branch might be missing is that the ContinueStmt is added after the OmpEndLoopDirective Evaluation. It might all work if it is before. I will give that a try.

[vdonaldson]

Yes, swapping the order of the ContinueStmt and the OmpEndLoopDirective is definitely worth a look.

[kiranchandramohan]

I am now using the constructExit to find the terminator block.

I tried to create the ContinueStmt before the OmpEndLoopDirective but that did not work. Not sure whether my approach for doing this was correct.

[vdonaldson]

Branch analysis should automatically set the isNewBlock flag, at least for looping constructs. Are there "straight-line" OpenMP constructs that aren't automatically setting this flag? (If yes, there is another option for getting an "exit" for it.) For looping constructs, if the ContinueStmt is not being marked as a new block, it is probably in the wrong place, perhaps with respect to an OmpEndLoopDirective eval.

[kiranchandramohan]

For looping constructs, the ContinueStmt is marked as a new block. It is for the outer parallel construct that the ContinueStmt is not marked as a new block. Due to this, previously, they end up sharing the same block when createEmptyRegionBlocks is called for both.

[vdonaldson]

If I understand how code should look for parallel regions, not all inserted ContinueStmts are branch targets. Unstructured constructs need not have branches that exit the construct. A loop could be unstructured due to the presence of a simple goto entirely local to the loop body, or some nested construct with an ExitStmt. I believe all basic block recognition is currently local to PFT routine analyzeBranches.

That said, if these ContinueStmts must always be in their own (unstructured) basic block, or you can at least determine that forcing such a block all the time is not a correctness problem, then it might be ok to initially force creation of such blocks all the time, with the option of going back later to sharpen up the code to only set it when needed. The -no-structured-fir flag should make that testing a little more robust.

It should be easier to investigate the problem once PR #1399 is available; thanks for splitting that out.

[kiranchandramohan]

I have now removed the requirement for ContinueStmts to be always in their own basic block. This became possible after restricting createEmptyRegionBlocks to not recurse into nested OpenMPConstructs/Directives.

[kiranchandramohan]

It might be a good idea to split off the cherry pick changes that avoid the buggy error message. That fix is a prerequisite to, and independent of other fixes.

Done. Created a separate PR #1399.

You can get additional testing of unstructured code by forcing unstructured code generation for all loops with the -no-structured-fir developer flag. The intent is to make it easier to exercise less common unstructured code cases without actually having to put in unstructured branches.

OK. will try this.

[kiranchandramohan]

@vdonaldson @Leporacanthicus After investigating a bit further I found out that we only have to avoid creating empty blocks multiple times to fix the issues. This is achieved by checks for combined constructs (!$omp parallel do) and nested constructs (parallel, do). This simplifies the code a lot.

[vdonaldson]

This set of changes looks good to me. It lets the normal control flow machinery do its usual work, and keeps directive-centric changes in directive-centric code.

Thanks for digging into this.