[PATCH] D88221: [flang][OpenMP] Upstream OpenMP Parallel construct codegen support

Thu Sep 24 08:01:33 PDT 2020

jdoerfert added a comment.

In D88221#2292676 <https://reviews.llvm.org/D88221#2292676>, @SouraVX wrote:

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

I'm saying the commit subject and message should describe the code change. Currently, the subject and message describe the PR (I guess) but not this commit.

In D88221#2292675 <https://reviews.llvm.org/D88221#2292675>, @SouraVX wrote:

> In D88221#2292669 <https://reviews.llvm.org/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"?
>
> Unfortunately only a small chunk can be upstreamed, so some where context is getting lost. This PR(mentioned) does the entire "CodeGen".
>
>> Also "Code inside a Parallel operation region is lowered into FIR Dialect" seem to not capture this, this is just the "overall phase" in which the code is executed, right?
>> I mean, what happens here is the insert point is moved from something to something. Can we add a sentence that says why the new location is better? Maybe add a test?
>
> Regression test capturing some statements `if` and `print` statements(with end-to-end functionally tested)has been added to PR. Again unfortunately those also can't be added here. Since `Lowering Code` still lives in downstream/upstream `FIR` dev.

This is indeed unfortunate, to say the least.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D88221