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

[Bataev, Alexey]

Ok, I'll add it

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

25.09.2014 18:28, 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 10:58:49 PM
>> Subject: Re: [PATCH] [OPENMP] Codegen for 'firstprivate' clause in 'parallel' directive
>>
>> 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
> Okay, good to know. Please encode this information in a comment somewhere.
>
>   -Hal
>
>> 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
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>