As with regular calls, we want to specialize a call that went through
template instantiation if it has an applicable OpenMP declare variant.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Unit Tests
Event Timeline
clang/lib/Sema/SemaTemplateInstantiate.cpp | ||
---|---|---|
1698 | Is there a parent change that removed the Sema& parameter from this function? |
clang/lib/Sema/SemaTemplateInstantiate.cpp | ||
---|---|---|
1698 | 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. |
clang/lib/Sema/SemaTemplateInstantiate.cpp | ||
---|---|---|
1693 | I just didn't know about it. I'll move the code there. |
clang/lib/Sema/SemaExprCXX.cpp | ||
---|---|---|
8260 ↗ | (On Diff #255524) | 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. |
clang/lib/Sema/SemaExprCXX.cpp | ||
---|---|---|
8260 ↗ | (On Diff #255524) | 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. |
Override RebuildCallExpr in the template instantiation and specialize calls only there.
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.
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. |
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. |
Reuse ActOnCallExpr, which contains the necessary logic (and a warning which we tried to avoid before).
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).
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?