[SYCL] Define templates used in multiple Sema TUs in a common .h file #1164
Conversation
| @@ -8,6 +8,8 @@ | |||
| // This file implements C++ template instantiation for declarations. | |||
| // | |||
| //===----------------------------------------------------------------------===/ | |||
| #include "SemaTemplateImpl.h" | |||
There was a problem hiding this comment.
Are template methods from new header are used in this source file?
If not, please, remove this include.
There was a problem hiding this comment.
Alexey, they are. The first one - AddOneConstantValueAttr - is used at around lines 528 and 539. The second one - AddOneConstantPowerTwoValueAttr - is used at around lines 506 and 517.
There was a problem hiding this comment.
How is this working today if these are defined in other source file?
Are we instantiating the same specializations in both files?
How did this code pass code review?
There was a problem hiding this comment.
It works today because when the files are linked together, the definition is found from the other file. The declaration is seen from Sema.h, so it compiles fine. The reason the issue was found is that we have a more optimizing build where the template definition got completely inlined, and the other file couldn't find the definition and failed at link-time.
Yes, we are instantiating the same specialization in both files.
There was a problem hiding this comment.
Why not define these methods in Sema.h then?
There was a problem hiding this comment.
I don't think there is a technical reason. I would rather not have implementations there to reduce the clutter in that file.
Do you feel strongly about this?
There was a problem hiding this comment.
I think it would be hard to debug cases when Sema.h is included and methods defined in a separate header is used.
Reducing clutter in Sema.h shouldn't be resolved by this PR. Let's handle it separately.
|
Please note: that is not the only place where templated declarations are handled. Are you sure, that you want to process this refactoring? |
This was the only place that we found an issue with definitions in source files (which were used in another source file), which is what motivated this particular refactoring. |
There was a problem hiding this comment.
@premanandrao, please, move Sema member function templates definitions to Sema.h header file.
premanandrao
force-pushed
the
common_definition
branch
2 times, most recently
from
d175857 to
6596d10
Compare
Signed-off-by: Premanand M Rao <premanand.m.rao@intel.com>
premanandrao
force-pushed
the
common_definition
branch
from
6596d10 to
6fa3a2e
Compare
…ttributes (#811) Signed-off-by: Viktoria Maksimova <viktoria.maksimova@intel.com>
intel#6601 increases number of captured variable and thus number of kernel arguments for reductions. Modify the CHECKs accordingly.
Signed-off-by: Premanand M Rao premanand.m.rao@intel.com