[PATCH] [OPENMP] Codegen for 'firstprivate' clause in 'parallel' directive

[Bataev, Alexey]

Hal, I think "declared here" messages should point to the real 
declaration of the variable. "firstprivate" is an implicit declaration. 
Besides, if the original variable is not used in the capturedstmt 
construct it won't be captured at all and I won't be able to use its 
value in the OpenMP construct for initialization

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

25.09.2014 7:23, Hal Finkel пишет:
> ----- Original Message -----
>> From: "Alexey Bataev" <a.bataev at hotmail.com>
>> To: "Hal Finkel" <hfinkel at anl.gov>
>> Cc: cfe-commits at cs.uiuc.edu, dgregor at apple.com, fraggamuffin at gmail.com, richard at metafoo.co.uk, rjmccall at gmail.com,
>> ejstotzer at gmail.com, reviews+d5140+public+cb73bc52a9cc08f9 at reviews.llvm.org
>> Sent: Wednesday, September 24, 2014 9:45:56 PM
>> Subject: Re: [PATCH] [OPENMP] Codegen for 'firstprivate' clause in 'parallel' directive
>>
>>> It does not look like you call Sema::PushOnScopeChains with the new
>>> private variable declaration. Why?
>> Because this variable is not used in Sema, so the resolver does not
>> need
>> to look for this private variable. Actual replacing of the original
>> variable by the private one occurs in codegen.
> I figured as much, but why is that the design you've chosen? Would it be better to have Sema use that variable? If nothing else, it might yield better 'declared here' notes on errors/warnings (or worse ones, depending on whether or not you want the 'declared here' notes to point to the firstprivate/private clauses or not).
>
>   -Hal
>
>> Best regards,
>> Alexey Bataev
>> =============
>> Software Engineer
>> Intel Compiler Team
>>
>> 24.09.2014 18:09, Hal Finkel пишет:
>>> ----- Original Message -----
>>>> From: "Alexey Bataev" <a.bataev at hotmail.com>
>>>> To: "Hal Finkel" <hfinkel at anl.gov>
>>>> Cc: cfe-commits at cs.uiuc.edu, dgregor at apple.com,
>>>> fraggamuffin at gmail.com, richard at metafoo.co.uk,
>>>> rjmccall at gmail.com,
>>>> ejstotzer at gmail.com,
>>>> reviews+d5140+public+cb73bc52a9cc08f9 at reviews.llvm.org
>>>> Sent: Wednesday, September 24, 2014 2:34:53 AM
>>>> Subject: Re: [PATCH] [OPENMP] Codegen for 'firstprivate' clause in
>>>> 'parallel' directive
>>>>
>>>> Hal, All these changes are part of codegen. I replaced our temp
>>>> checks
>>>> for firstprivate vars to the proper one.
>>>> Private copies are required for correct codegen of firstprivate
>>>> vars
>>>> with constructors/destructors and for correct debug info (if you
>>>> try
>>>> to
>>>> debug code generated by clang from clang-omp.github.io you will
>>>> have
>>>> a
>>>> lot of troubles with it, because there is no correct debug info
>>>> for
>>>> private vars
>>> Yes, I've noticed this ;)
>>>
>>>> ).
>>>> For each firstprivate var we're generating a private copy inside
>>>> an
>>>> OpenMP region that will be used instead of the original var. Then
>>>> we're
>>>> generating an intializer for these private vars with the value of
>>>> the
>>>> corresponding original variable. Also we need some additional
>>>> helper
>>>> variables if the firstprivate variable has an array type. In this
>>>> case
>>>> we're generating initializer for a single item of that array and
>>>> in
>>>> codegen use this initializer for proper array initialization.
>>> To some extent, I understand. firstprivate(i) is semantically like
>>> a declaration of a new variable i that is initialized with the old
>>> variable i. It does seem similar to what is done for
>>> explicitly-named lambda capture variables (in
>>> Sema::ActOnStartOfLambdaDefinition where it calls
>>> Sema::createLambdaInitCaptureVarDecl). It does not look like you
>>> call Sema::PushOnScopeChains with the new private variable
>>> declaration. Why?
>>>
>>> Thanks again,
>>> Hal
>>>
>>>> Best regards,
>>>> Alexey Bataev
>>>> =============
>>>> Software Engineer
>>>> Intel Compiler Team
>>>>
>>>> 23.09.2014 16:56, Hal Finkel пишет:
>>>>> ----- Original Message -----
>>>>>> From: "Alexey Bataev" <a.bataev at hotmail.com>
>>>>>> To: "a bataev" <a.bataev at hotmail.com>, dgregor at apple.com,
>>>>>> fraggamuffin at gmail.com, richard at metafoo.co.uk,
>>>>>> rjmccall at gmail.com, ejstotzer at gmail.com, hfinkel at anl.gov
>>>>>> Cc: cfe-commits at cs.uiuc.edu
>>>>>> Sent: Monday, September 22, 2014 11:26:56 PM
>>>>>> Subject: Re: [PATCH] [OPENMP] Codegen for 'firstprivate' clause
>>>>>> in
>>>>>> 'parallel' directive
>>>>>>
>>>>>>> Please separate out the Sema changes from this patch into a
>>>>>>> separate patch.
>>>>>> Hal, the changes in Sema are required for codegen only. I won't
>>>>>> be
>>>>>> able to test them without codegen.
>>>>>>
>>>>> Okay, obviously there are changes to the Sema tests because the
>>>>> messages have changed. Can you commit that change separately? If
>>>>> this is not reasonable/possible, please explain why.
>>>>>
>>>>> Also, regarding the addition of the '*PrivateCopies' functions to
>>>>> the AST nodes, can you please explain the design? I don't
>>>>> understand why you seem to be duplicating some of the variables
>>>>> in
>>>>> the AST.
>>>>>
>>>>> Thanks again,
>>>>> Hal
>>>>>
>>>>> [snip]
>>>>>
>>>>>> http://reviews.llvm.org/D5140
>>>>>>
>>>>>>
>>>>>>
>>