This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Allow traits for the OpenMP context selector `isa`
ClosedPublic

Authored by jdoerfert on Jul 6 2020, 11:14 PM.

Details

Summary

It was unclear what isa was supposed to mean so we did not provide any
traits for this context selector. With this patch we will allow *any*
string or identifier. We use the target attribute and target info to
determine if the trait matches. In other words, we will check if the
provided value is a target feature that is available (at the call site).

Fixes PR46338

Diff Detail

Event Timeline

jdoerfert created this revision.Jul 6 2020, 11:14 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJul 6 2020, 11:14 PM

Agreed on tests. I like the mechanism - passing a string through to the backend as a way to dispatch between isa properties looks cleanly extensible. We probably do want to emit a warning when the backend claims it doesn't know anything about said string as it'll be prone to typos.

jdoerfert updated this revision to Diff 278175.Jul 15 2020, 7:06 AM

Add warning and negative test

jdoerfert edited the summary of this revision. (Show Details)Jul 15 2020, 7:06 AM
ABataev added inline comments.Jul 23 2020, 8:37 AM
clang/include/clang/AST/OpenMPClause.h
7641

No need for the default initializer here

7700

final and virtual default destructor?

7707

override?

clang/lib/Parse/ParseOpenMP.cpp
1829

Seems to me, SourceLocation is usually captured by value.

clang/lib/Sema/SemaOpenMP.cpp
5898–5906

CE can be captured by value

llvm/lib/Frontend/OpenMP/OMPContext.cpp
205

llvm::all_of?

jdoerfert updated this revision to Diff 280148.Jul 23 2020, 8:48 AM
jdoerfert marked 6 inline comments as done.

Addressed @ABataev comments, thx!

ABataev added inline comments.Jul 23 2020, 8:51 AM
clang/include/clang/AST/OpenMPClause.h
7705

virtual

jdoerfert updated this revision to Diff 280151.Jul 23 2020, 8:53 AM
jdoerfert marked an inline comment as done.

Add virtual

minor fixes

ABataev accepted this revision.Jul 29 2020, 6:18 AM

LG with a nit.

clang/lib/AST/OpenMPClause.cpp
2304–2306

It

This revision is now accepted and ready to land.Jul 29 2020, 6:18 AM
This revision was landed with ongoing or failed builds.Jul 29 2020, 8:24 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Jul 29 2020, 10:05 AM

This breaks check-clang on Windows: http://45.33.8.238/win/20850/step_7.txt

Please take a look, and revert while you investigate if it takes a while to fix.

This breaks check-clang on Windows: http://45.33.8.238/win/20850/step_7.txt

Please take a look, and revert while you investigate if it takes a while to fix.

Should be fixed by 8723280b68b1e5ed97a699466720b36a32a9e406, apologies for the delay.

Looks like it's still failing with that change: http://45.33.8.238/win/20885/step_7.txt

Looks like it's still failing with that change: http://45.33.8.238/win/20885/step_7.txt

dso_local is *before* the void,.. 2 min

That did the trick, thanks!

The bot was broken for > 5h and 43 builds. We got lucky that nothing else broke in the meantime, but red bots make it more difficult to analyze new regressions that appear when the bots are already red. Next time, please revert while you investigate / until you have time to investigate.

mikerice added inline comments.
clang/include/clang/AST/OpenMPClause.h
7641

This field doesn't seem to have serialization code. Is that expected or an oversight?

jdoerfert added inline comments.Nov 19 2020, 4:55 PM
clang/include/clang/AST/OpenMPClause.h
7641

Oversight. I assume we need it to preserve the behavior when we go the pch route.