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

[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
> >>>>>>
> >>>>>>
> >>>>>>
> >>
> 
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory