This is an archive of the discontinued LLVM Phabricator instance.

[OpenMP] Specialize OpenMP calls after template instantiation
ClosedPublic

Authored by jdoerfert on Apr 2 2020, 12:21 AM.

Details

Summary

As with regular calls, we want to specialize a call that went through
template instantiation if it has an applicable OpenMP declare variant.

Diff Detail

Unit TestsFailed

Event Timeline

jdoerfert created this revision.Apr 2 2020, 12:21 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 2 2020, 12:21 AM
mikerice added inline comments.
clang/lib/Sema/SemaTemplateInstantiate.cpp
1698 ↗(On Diff #254438)

Is there a parent change that removed the Sema& parameter from this function?

jdoerfert marked an inline comment as done.Apr 3 2020, 1:42 PM
jdoerfert added inline comments.
clang/lib/Sema/SemaTemplateInstantiate.cpp
1698 ↗(On Diff #254438)

Yes, locally. I was going to submit is as a NFC patch later today. The parameter was a leftover I simply forgot to remove when I made it a member.

jdoerfert updated this revision to Diff 255043.Apr 4 2020, 8:44 AM
jdoerfert edited the summary of this revision. (Show Details)

Rebase on D77252

mikerice added inline comments.Apr 4 2020, 9:55 AM
clang/lib/Sema/SemaTemplateInstantiate.cpp
1693 ↗(On Diff #255043)

Is there a reason you are adding this here as opposed to in the base class RebuildCallExpr? Are there cases where we rebuild call expressions that we don't want to do this variant processing?

1698 ↗(On Diff #255043)

Comments on nullptr arguments would be nice.

jdoerfert marked an inline comment as done.Apr 5 2020, 8:39 PM
jdoerfert added inline comments.
clang/lib/Sema/SemaTemplateInstantiate.cpp
1693 ↗(On Diff #255043)

I just didn't know about it. I'll move the code there.

jdoerfert updated this revision to Diff 255524.Apr 6 2020, 3:48 PM

Move code as suggested

mikerice added inline comments.Apr 6 2020, 5:34 PM
clang/lib/Sema/SemaExprCXX.cpp
8260

I was expecting the code would be added to RebuildCallExpr in TreeTransform.h. This seems to just override one of the classes derived from TreeTransform.

I think I'd try adding it to TreeTransform then check all the cases where a subclass overrides RebuildCallExpr and try to determine if something needs to be done there.

jdoerfert marked an inline comment as done.Apr 6 2020, 6:15 PM
jdoerfert added inline comments.
clang/lib/Sema/SemaExprCXX.cpp
8260

I'm not paying attention -.-

This is the one class that overrides RebuildCallExpr in clang and obviously not the right place for this. I will put it in TreeTransform::RebuildCallExpr which is in clang/lib/Sema/TreeTransform.h now.

jdoerfert updated this revision to Diff 255560.Apr 6 2020, 6:22 PM

Override RebuildCallExpr in the template instantiation and specialize calls only there.

jdoerfert added a comment.EditedApr 6 2020, 6:23 PM

I figured in TreeTransform everyone would get this behavior but actually only template instantiation wants it so I put it there instead. Do you think it is better placed in the generic TreeTransform code after all?
At least this time I verified the test passes. Sorry for that btw.

EDIT: But the test system on phab seems to disagree. I'll check again.

My reasoning is that the generic TreeTransform does semantic analysis on CallExpr and this seems to be related semantic analysis that would need to be redone when the call changed as well. Say someone codes up a new subclass of TreeTransform that maybe changes one of the arguments. If ActOnOpenMPCall is not called does that create a broken AST?

So it is not clear to me that only template instantiation wants it. I don't claim to be an expert on this code but if I subclassed TreeTransform I wouldn't expect to have to rewrite this OpenMP code each time I modified the base call expression. We could probably use more opinions on this in any case.

jdoerfert updated this revision to Diff 255709.Apr 7 2020, 10:06 AM

Move specializtion to the TreeTransformer

erichkeane added inline comments.
clang/lib/Sema/TreeTransform.h
2414 ↗(On Diff #255709)

It seems to me that this logic would better fit in 'ActOnCallExpr' if at all possible, and RebuildCallExpr should just call ActOnCallExpr. I see there is a warning implemented there (a strange place for that), but if this doesn't cause conflicts I would expect that to be the behavior here.

2418 ↗(On Diff #255709)

What would BuildCallExpr return that is both valid/usable and NOT a callexpr? I'm not sure this check needs to be here.

jdoerfert marked an inline comment as done.Apr 7 2020, 10:45 AM
jdoerfert added inline comments.
clang/lib/Sema/TreeTransform.h
2414 ↗(On Diff #255709)

The logic is in ActOnCallExpr so if I can call that here it should work just fine.

jdoerfert updated this revision to Diff 255729.Apr 7 2020, 10:48 AM

Reuse ActOnCallExpr, which contains the necessary logic (and a warning which we tried to avoid before).

erichkeane accepted this revision.Apr 7 2020, 10:56 AM

From the CFE perspective, all this looks OK to me. But please give @mikerice a couple hours before committing to do a final set of comments to make sure the OMP examples are sufficient for him (or any final objections).

This revision is now accepted and ready to land.Apr 7 2020, 10:56 AM
mikerice accepted this revision.Apr 7 2020, 11:03 AM

This looks like the right change to me.

This revision was automatically updated to reflect the committed changes.