[cfe-commits] [PATCH] First OpenMP patch

[Hal Finkel]

----- Original Message -----
> From: "Chandler Carruth" <chandlerc at google.com>
> To: "Hal Finkel" <hfinkel at anl.gov>
> Cc: "Dmitri Gribenko" <gribozavr at gmail.com>, "benny kra" <benny.kra at gmail.com>, "Alexey Bataev" <a.bataev at gmx.com>,
> "llvm cfe" <cfe-commits at cs.uiuc.edu>
> Sent: Monday, December 17, 2012 8:42:03 AM
> Subject: Re: [cfe-commits] [PATCH] First OpenMP patch
> 
> 
> 
> On Mon, Dec 17, 2012 at 5:45 AM, Hal Finkel < hfinkel at anl.gov >
> wrote:
> 
> 
> 
> 
> 
> ----- Original Message -----
> > From: "Dmitri Gribenko" < gribozavr at gmail.com >
> > To: "Alexey Bataev" < a.bataev at gmx.com >
> > Cc: "Hal Finkel" < hfinkel at anl.gov >, cfe-commits at cs.uiuc.edu ,
> > "mahesha llvm" < mahesha.llvm at gmail.com >, "benny kra"
> > < benny.kra at gmail.com >
> > Sent: Monday, December 17, 2012 6:45:07 AM
> > Subject: Re: [cfe-commits] [PATCH] First OpenMP patch
> > 
> 
> > On Mon, Dec 17, 2012 at 11:48 AM, Alexey Bataev < a.bataev at gmx.com
> > >
> > wrote:
> > > Dmitry, Hal,
> > > Thank you for your comments and sorry for the delay, I was on a
> > > vacation.
> > > I've made some fixes according to your comments.
> > > Hal, I've change the sentence in the doc. Now there is only one
> > > warning, if
> > > any OpenMP pragma is found.
> > > Dmitry, I've changed the processing of the -fno-openmp flag. If
> > > -fno-openmp
> > > is specified, the option -Wno-source-uses-openmp is passed to the
> > > frontend.
> > > Option -Wsource-uses-openmp is on by default.
> > 
> > Hello Alexey,
> > 
> > This patch looks good to me.
> 
> This also looks good to me. In nobody objects in the next day or so,
> please commit.
> 
> 
> Sorry that this got lost Hal, but I have said on another thread about
> this patch (but with a different author) that I don't really think
> we should add documentation and the beginnings of support for
> -fopen-mp without first having a clear discussion and document
> describing the expected design of OpenMP support in Clang.

Regarding the documentation, I agree. It would be better to leave it out at this point (we probably don't want docs for such an incomplete feature to appear on the web site at present), although it should still be developed.

Regarding the -fopenmp flag, I disagree. For one thing, clang already has support for this flag. It already appears in clang/Driver/Options.td and is handled in Driver/Tools.cpp. The largest thing that we lack is testing coverage, and this patch adds that. Also, as part of this discussion, we've decided how we want this flag to act, which is also good, and the right thing to do, IMHO, along with adding testing coverage.

> 
> 
> Essentially, I think this patch is starting ast step 2, 3, or 4
> rather than step 1.
> 
> 
> I think we're actually imagining OpenMP working generally the same
> way these days (based on the discussion we had on the aforementioned
> thread -- another reason I really dislike forking threads), but I'd
> like to get that documented, and a general roadmap of how the
> support is going to be added to Clang and by whom laid out first.

I think this discussion of the command-line flags is sufficiently separate from the AST design and other handling that having it be separate is actually a good thing. Nevertheless, I see your point.

> And I think based on *that* discussion, Doug needs to sign off on
> OpenMP as a viable language extension for Clang to support. I think
> it is viable, and would support it, but it's not obvious under the
> current rules. Does this make sense to folks? I think it's the next
> step.

For one thing, there are already TODO's regarding OpenMP in Parse/ParseStmt.cpp; those, along with the preexisting -fopenmp flag support, and previous discussions with various people, led me to believe that this decision, at least to some extent, had already been made. Nevertheless, we certainly do need a higher-level discussion.

Doug, how should we structure the discussion regarding OpenMP support? Are you okay with the changes in this patch specifically?

> 
> 
> 
> 
> Finally, I would point out that I still have some concerns over the
> contributors of OpenMP support not being significant contributors to
> Clang in general, and not contributing to the support and
> maintenance burden of the system as a whole. I have not yet seen
> significant changes there, although I'm hopeful they'll be
> forthcoming. I think that is an essential component to getting these
> patches in and adding OpenMP to the Clang.

I think that this concern is fair, but I would recommend withholding judgment. As more work is done, more naturally-bordering maintenance tasks will beg for attention ;)

Thanks again,
Hal

-- 
Hal Finkel
Postdoctoral Appointee
Leadership Computing Facility
Argonne National Laboratory