Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Avoid migrating existing patches. Phabricator shutdown timeline

[OpenMP][Clang] Allow passing target features in ISA trait for metadirective clause
ClosedPublic

Authored by saiislam on Jan 3 2022, 10:46 AM.

Details

Summary

Passing any feature in the device-isa trait which is not supported by the host
was causing a compilation failure.

Diff Detail

Event Timeline

saiislam created this revision.Jan 3 2022, 10:46 AM
saiislam requested review of this revision.Jan 3 2022, 10:46 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 3 2022, 10:46 AM

Can you make the test check for the diagnose message? Also, do we have a test to verify an isa trait is properly handled?

clang/lib/Parse/ParseOpenMP.cpp
2555

Why doesn't this diagnose nothing?

Can you make the test check for the diagnose message? Also, do we have a test to verify an isa trait is properly handled?

I don't see a point in adding a diagnostic message when a feature is not found valid for a certain target. Because if it satisfies one of the host or device compilation, then it will definitely fail in the other.
I have added new tests to check that isa trait is handled properly in the next revision.

clang/lib/Parse/ParseOpenMP.cpp
2555

Because an isa-feature will fail at least once, for either host compilation or device compilation. So, no point in always giving a warning.

saiislam updated this revision to Diff 397912.Jan 6 2022, 8:40 AM

Added target specific tests for ISA traits, for CPU as well as GPU.

jdoerfert added inline comments.Jan 6 2022, 9:58 AM
clang/lib/Parse/ParseOpenMP.cpp
2555

That is debatable.

First, if I compile for a single architecture there is no device compilation and it should warn.
Second, if I place the metadirective into a declare variant function or add a kind(...) selector to it it will also not warn even if you have multiple architectures.

saiislam updated this revision to Diff 398179.Jan 7 2022, 9:42 AM
saiislam marked an inline comment as done.

Added diagnostic remarks for when ISA trait is not selected.

saiislam updated this revision to Diff 398614.Jan 10 2022, 6:41 AM

Fixed the lit test failing in pre-check build bot.

jdoerfert added inline comments.Jan 10 2022, 6:45 AM
clang/lib/Parse/ParseOpenMP.cpp
2555
ASTContext &Context = getASTContext();
std::function<void(StringRef)> DiagUnknownTrait = [this,
┊ ┊ ┊ ┊ ┊ ┊ ┊ ┊ ┊ ┊                                CE](StringRef ISATrait) {
┊ // TODO Track the selector locations in a way that is accessible here to
┊ // improve the diagnostic location.
┊ Diag(CE->getBeginLoc(), diag::warn_unknown_declare_variant_isa_trait)
┊ ┊ ┊ << ISATrait;                                                   
};
TargetOMPContext OMPCtx(Context, std::move(DiagUnknownTrait),                                                                                                                                                                              
┊ ┊ ┊ ┊ ┊ ┊ ┊ ┊ ┊ ┊     getCurFunctionDecl(), DSAStack->getConstructTraits());

Already exists (SemaOpenMP). Why do we need a second, different diagnostic?

saiislam added inline comments.Jan 10 2022, 7:06 AM
clang/lib/Parse/ParseOpenMP.cpp
2555

Isn't giving a remark better than a warning, when we know in many cases this will be hit during a normal (expected) compilation for target offload?
Remark diagnostic will give ample handle for understanding the flow without the need to explicitly deal with this warning during compilation of user programs.

I am fine changing it to a warning if you feel strongly about this.

jdoerfert added inline comments.Jan 10 2022, 2:18 PM
clang/lib/Parse/ParseOpenMP.cpp
2555
  1. Having two diagnostics for the same thing is generally bad and there doesn't seem to be a reason why we would need/want to treat metadirective and declare variant differently. Thus, if we want to move to remarks we should move the second diagnostic as well. Cleaning up existing code is more important than adding new code.
  2. I still don't see why this is "normal" or "expected" at all. The warning reads:

isa trait 'foo' is not known to the current target; verify the spelling or consider restricting the context selector with the 'arch' selector further.
Hence, device={isa("flat..."), arch(amdgcn)} should not cause a warning even if you compile it for the host, an nvidia gpu, or anything other than AMDGCN.
FWIW, this is [supposed to be] ensured by the positioning of the isa trait selector in OMPKinds.def:

// Note that we put isa last so that the other conditions are checked first.
// This allows us to issue warnings wrt. isa only if we match otherwise.
__OMP_TRAIT_SELECTOR(device, isa, true)
saiislam updated this revision to Diff 398897.Jan 11 2022, 3:51 AM
  1. Used a common diagnostic warning warn_unknown_declare_variant_isa_trait for ParseOpenMP and SemaOpenMP for decalre variant and metadirectives.
  2. Split lit codegen tests into two files, one requiring amdgpu-registered target and another for host only.
  3. Added warning message lit test at an appropriate place.
saiislam marked an inline comment as done.Jan 11 2022, 3:51 AM
jdoerfert accepted this revision.Jan 11 2022, 7:33 AM

LG, thanks for the adjustment. Hope you are happy with the result.

This revision is now accepted and ready to land.Jan 11 2022, 7:33 AM
This revision was landed with ongoing or failed builds.Jan 11 2022, 9:26 PM
This revision was automatically updated to reflect the committed changes.