Page MenuHomePhabricator

[OpenMP] Add support for omp simd pragmas without runtime
AbandonedPublic

Authored by huntergr on Mar 28 2017, 3:22 AM.

Details

Summary

Adds a new flag ('-fopenmp-simd') to clang which enables processing
of 'simd' and 'declare simd' pragmas without supporting the rest
of OpenMP.

The pragma handler will filter out directives and clauses which
aren't related to simd, and the driver will not add lib(g)omp to
the list of libraries to link.

Documentation updated to describe the new flag.

Diff Detail

Event Timeline

huntergr created this revision.Mar 28 2017, 3:22 AM

Hi Graham,

thank you for working on this. I understand that you are gonna take care of the CodeGen side of things for the new -fopenmp-simd option in a separate patch?

I am asking because I expect that the tests in test/OpenMP/declare_simd_* will give the same results when invoking clang with the new flag instead of -fopenmp.

Thank you,

Francesco

Hi Graham,

thank you for working on this. I understand that you are gonna take care of the CodeGen side of things for the new -fopenmp-simd option in a separate patch?

CodeGen for 'simd' pragmas works fine with this patch. 'declare simd' doesn't quite work yet, but yes, I was planning to do that in a separate patch.

I am asking because I expect that the tests in test/OpenMP/declare_simd_* will give the same results when invoking clang with the new flag instead of -fopenmp.

So 'isAllowedClauseForDirective' needs to be updated; at present, it just bails out immediately if the directive kind passed in is OMPD_declare_simd. The clauses for a 'declare simd' are handled a little differently in ParseOpenMP.cpp, and I need to look into why.

I mainly wanted feedback on the approach at this point, since the patch is pretty small and changes won't be too cumbersome.

Hi Graham,

I don't know much about Clang's machinery, but would it be possible to have -fopenmp-simd generate the same handler, but with restrictions? I fear this slight duplication could get considerably worse as we support more and more "non-RT" OMP pragmas.

Alternatively, if this is for testing purposes, we have another pragma which does exactly the same thing as omp simd, which are the Clang vectorizer pragmas (http://llvm.org/docs/Vectorizers.html#pragma-loop-hint-directives).

cheers,
--renato

Hi Renato,

I don't know much about Clang's machinery, but would it be possible to have -fopenmp-simd generate the same handler, but with restrictions? I fear this slight duplication could get considerably worse as we support more and more "non-RT" OMP pragmas.

Sure, I can combine the handlers and switch behaviour within that if that's preferable. The other alternative I thought of was to perform the filtering in ParseOpenMP.cpp instead, but I need to figure out how to delete or skip tokens there without cluttering up the rest of the OpenMP parsing.

I'll come up with a new version with the combined handler tomorrow.

Alternatively, if this is for testing purposes, we have another pragma which does exactly the same thing as omp simd, which are the Clang vectorizer pragmas (http://llvm.org/docs/Vectorizers.html#pragma-loop-hint-directives).

This feature comes from user requests, and basically matches the functionality of other compilers (e.g. gcc).

Thanks for the comments.

-Graham

The other alternative I thought of was to perform the filtering in ParseOpenMP.cpp instead, but I need to figure out how to delete or skip tokens there without cluttering up the rest of the OpenMP parsing.

That was my first thought, yes, but I'm not sure how invasive this could get.

This feature comes from user requests, and basically matches the functionality of other compilers (e.g. gcc).

Right, in this case, I think we have a stronger motivation to make that an integral part of the existing OpenMP parser.

cheers,
--renato

huntergr updated this revision to Diff 93841.Apr 3 2017, 4:40 AM
huntergr added a reviewer: kkwli0.

Changed to transform combined constructs to simd in ParseOpenMP.cpp instead of creating a new pragma handler. This also made it easier to add support for 'declare simd': only needed the addition of a check for the option when code generating functions to enable it, so I've added a RUN line to test it in the 'declare simd' codegen tests.

Hi Graham,

It looks much simpler now, thanks!

I'm ok with the change, but I'd like @ABataev to confirm that the semantics is the expected one for all cases and approve.

cheers,
--renato

lib/Parse/ParseOpenMP.cpp
174

I'd like @ABataev to confirm this is the right semantics.

1047

What if it's really unknown, even to -fopenmp-simd?

huntergr added inline comments.Apr 3 2017, 5:04 AM
lib/Parse/ParseOpenMP.cpp
174

Yes, would be good. I don't think there's a formal spec for this feature, but it's possible that directives intended for a different target than the cpu shouldn't apply with this flag.

1047

I did wonder about handling this case; I defaulted to ignoring it, since we are already filtering out other non-simd constructs.

If we do want to catch it, then I can think of two options: creating the diagnostic right before the filter switch (possibly messy), or adding a new enum value of OMPD_non_simd_construct (or a similar name) to represent constructs we recognize but don't want to handle and differentiate against a true unknown. I think the latter would be preferable.

ABataev added inline comments.Apr 6 2017, 8:56 AM
lib/CodeGen/CodeGenModule.cpp
122–123

I don't think you need to create OpenMP runtime support object if you're not going to use it (and you're not going to use it because you don't want to link it). You'd better create your own OMPSIMDRuntime class, that supports emission of the simd code only.

lib/Parse/ParseOpenMP.cpp
174

I don't think you need it here. Instead, you should keep an existing logic in Sema/Parsing code, but you need to create your own OpenMPRuntime support class, that supports only emission of simd part of the constructs.

1047

Leave it as is, we still have to report errors, even in simd-only mode

kkwli0 added inline comments.Apr 6 2017, 10:20 AM
docs/ClangCommandLineReference.rst
1454

I am not sure if it is target architecture specific or not. If it is, should we be under the target flag instead?

huntergr added inline comments.Apr 11 2017, 2:48 AM
lib/Parse/ParseOpenMP.cpp
174

Thanks for taking a look.

I tried this method first, but came to the conclusion it was too invasive. I've now tried it a second time and came to the same conclusion.

The problem isn't in generating 'simd' or 'declare simd' from a custom runtime class, but in generating everything else. Take a 'parallel for' directive -- I have to generate something close to unmodified code, but 'emitCommonOMPParallelDirective' in CGStmtOpenMP.cpp will only pass the loop codegen lambda through to the runtime class's 'emitParallelOutlinedFunction', which doesn't take the current CodeGenFunction as an argument (as it's expected to create a new one). There doesn't seem to be a good way to look up the CGF that the call will be made from.

You also won't get unmodified code in that instance anyway, as the loop is generated by CodeGenFunction::EmitWorkSharingLoop, and a cancellation region may be created; very little of this is under the control of the runtime class, so it would need extensive modification to work.

So I'll switch to using a simd-only 'runtime' class, but I still think we need to filter out non-simd directives before we get to codegen. Will post updated patch soon.

ABataev added inline comments.Apr 11 2017, 8:40 AM
lib/Parse/ParseOpenMP.cpp
174
  1. I don't think it is a good idea to skip the checks for non-simd constructs in this new mode. I'd prefer to emit all diagnostics as usual. Thus, we can't simply translate all simd-related constructs into 'omp simd' only, as we're going to loose checks/diagnostics.

Keep in minв that you will need to update all diagnostics-specific tests to check that the same errors/warnings are emitted even in -fopenmp-simd mode.

  1. You should not rewrite everything. You can do something like this:

file lib/CodeGen/CGStmt.cpp, function void CodeGenFunction::EmitStmt(const Stmt *S)
Insert before switch (S->getStmtClass()):

if (LangOpts.OpenMPSimd && isa<OMPExecutableDirective>(S)) {
  if (isOpenMPSimdDirective(S))
    EmitOMPSimdDirective(S);
  else
    EmitOMPInlinedDirective(S);
  return;
}

EmitOMPSimdDirective(S) shall emit the simd-specific code for all simd-related directives, while EmitOMPInlinedDirective(S) shall emit the code for all other constructs, ingnoring OpenMP directives.

hfinkel added a subscriber: hfinkel.Oct 2 2017, 4:18 PM

Is this still being worked on?

Is this still being worked on?

Hi, yes it is. Sorry for the delay in posting new changes but priorities shifted a bit and I had to work on something else for a while.

I do have a new version that's just about ready based on Alexey's idea. Simd constructs work, and non-simd constructs just pass the code straight through; combined constructs pose a bit of an issue, since they will have captured stmts from non-simd directives as well. Some additional work will be needed to handle that case. I'll post what I have after a bit of tidying for comments and suggestions; it's possible that the mixed case could be dealt with in another patch.

Is this still being worked on?

Hi, yes it is. Sorry for the delay in posting new changes but priorities shifted a bit and I had to work on something else for a while.

No problem.

I do have a new version that's just about ready based on Alexey's idea. Simd constructs work, and non-simd constructs just pass the code straight through; combined constructs pose a bit of an issue, since they will have captured stmts from non-simd directives as well. Some additional work will be needed to handle that case. I'll post what I have after a bit of tidying for comments and suggestions; it's possible that the mixed case could be dealt with in another patch.

Splitting patches in this way is generally a good idea.

The other thing I'd like in this regard is some kind of preprocessor feature that my users can use to test whether OpenMP SIMD is supported. Enabling -fopenmp-simd won't define _OPENMP (at least I suspect that it shouldn't because there might not be a meaningful <omp.h> to include). Maybe we should make #if __has_extension(openmp_simd) work?

I think we can abandon this one as support for -fopenmp-simd was committed already?

xtian accepted this revision.Mon, Apr 1, 2:20 PM

LGTM. This provides a consistent behavior same as GCC and ICC w/ -fopenmp-simd option. To answer, Kelvin's question. it is not directly tied with "target".

This revision is now accepted and ready to land.Mon, Apr 1, 2:20 PM
huntergr abandoned this revision.Tue, Apr 2, 1:44 AM

LGTM. This provides a consistent behavior same as GCC and ICC w/ -fopenmp-simd option. To answer, Kelvin's question. it is not directly tied with "target".

Thanks for the review Xinmin, but as Alexey notes in the previous comment -fopenmp-simd support has already been committed -- I missed that comment earlier, but should have closed it then.

Apologies for the delay in implementing it Alexey, I just had too many other things to take care of at the time, and thanks for getting a better implementation in.