[PATCH] [OPENMP] Codegen for 'if' clause

[Alexey Bataev]

Hello John, Thanks you very much for the review! Here are my answers.
> I apologize for the outrageous delays; we’ve been a little busy.
Oh, don't worry. I was just afraid that you're completely done with the 
clang because of swift. :-)

> The preconditions on this method aren’t clear.  Does it *assume* that there’s
> only one clause or does it *check*?  Right now, it seems to be assuming that
> it doesn’t have more than one, but checking that it doesn’t have zero, which
> is weird.
The precondition is quite simple: there must be exactly 0 or 1 clause of 
the specified kind. This method returns the single clause if it exists. 
Otherwise, if there is no such clause, it just returns nullptr.

> Elsewhere in the code, you’re conditionally checking whether the
> result is null.  Is there a language rule that you can only have one if clause?
This method is required for clauses llike "if", "num_threads" etc. These 
clauses may be specified only once per directive, but also they can be 
skipped, if they are not required. So, directives may have 0 or 1 
instance of such clauses.

> Just dereference the iterator here.  Your assert can then increment the
> original iterator, and you don’t have to worry about copying the iterator in
> non-asserts builds.
Done, thanks.

> This subclass should go in a separate, OpenMP-specific header.
Ok, moved to CGOpenMPRuntime.h

> You seem to be inconsistent about the capitalization here, and on reflection,
> honestly, I think this abbreviation is completely illegible.  And I’m not sure
> that calling it the “global thread ID” is actually meaningful when it’s clearly
> not global in any real sense.
>
> Let’s take everywhere you’ve got “Gtid” or “GTid”, including in the code
> already committed, and rename this to “ThreadID”.  So this would be:
>    const VarDecl *getThreadIDVariable() const { return ThreadIDVar; }
>
Ok, done.

> Also, this would be a great place to put a comment explaining what this
> variable actually is.  For example, that it’s a variable of type ident_t*.
I'll modify the comment. The type of this variable is kmp_int32, i.e. 
just int32.

> If this is an invariant of the CGOpenMPRegionInfo, shouldn’t it be asserted
> on in that structure, maybe in its constructor?
Moved to constructor.

> You have this logic repeated several times.  It would be reasonable
> to have a
>    LValue getThreadIDVariableLValue(CodeGenFunction &CGF);
> helper on the region info.
>
> Also, you seem to vary between using the type of the variable and assuming
> that ident_t == int32_t.  It’s probably better to trust the type of the variable
> consistently.
>
Created this function, replaced the code by function call.
The type of ThreadID is int32_t, not ident_t. ident_t is a structure 
that has info about debug location and some internal flags, ThreadID is 
just a 32-bit integer. The variable is declared only in outlined 
parallel region.
If the construct is not in parallel region (for example, there is a 
standalone '#omp atomic' or other non-parallel directive), this variable 
does not exist and we have to create a temp with type int32_t for this var.

> Please add a comment describing the intended behavior of this function and
> how its result will be used.  The rule seems to be “if we’re inside a captured
> statement, use the region info’s thread-ID variable and ignore the thread ID
> we just computed; otherwise, stash that thread ID in a temporary and return
> the address of that”.  That is a very strange rule.  Describing *why* it does
> that would be invaluable to people trying to maintain this in the future.
>
Ok, I'll add the description.

> I’m not sure what rule you’re following for what goes in CGOpenMP vs.
> CGOpenMPRuntime.  All of the code you’re emitting here is very
> runtime-specific.  I think you could very easily abstract out a virtual
> interface for doing runtime-specific logic, GCXXABI-style.  e.g. “emit
> a function for this captured statement”, “run this captured statement
> serially”, “run this captured statement in parallel”, etc.  All of that could
> be in CGOpenMPRuntime.cpp, and you could keep the general logic for
> interpreting down statements in CGOpenMP.cpp.
I'll look at GCXXABI and will try do my best to improve CodeGen for 
OpenMP, thanks.

> It would be better for this to not be templated and take a std::function.
Will do.

Best regards,
Alexey Bataev
=============
Software Engineer
Intel Compiler Team

17.09.2014 12:36, John McCall пишет:

http://reviews.llvm.org/D4716