Page MenuHomePhabricator

[OpenCL] Improve diagnostics for address spaces in template instantiation
ClosedPublic

Authored by Anastasia on Nov 23 2018, 6:07 AM.

Details

Summary

When template instantiation leads to creation of invalid code patterns with address spaces compiler crashes with ICE.

This patch fixes the template instantiation to provide correct diagnostics instead.

Diff Detail

Repository
rL LLVM

Event Timeline

Anastasia created this revision.Nov 23 2018, 6:07 AM
Anastasia updated this revision to Diff 175120.Nov 23 2018, 6:10 AM

Removed FIXME from the test case that is being fixed.

rjmccall added inline comments.Nov 26 2018, 11:16 AM
lib/Sema/TreeTransform.h
4241 ↗(On Diff #175120)

When do you actually add the qualifier back?

Also, I don't think this is specific to either OpenCL or direct references to template type parameters; it has to be any dependent type.

Anastasia marked an inline comment as done.Nov 27 2018, 11:49 AM
Anastasia added inline comments.
lib/Sema/TreeTransform.h
4241 ↗(On Diff #175120)

As far as I understand the qualifiers here are only used to rebuild the type. Therefore I assumed I don't need to restore the original. I am now thinking of moving this down into RebuildQualifiedType that has similar code for handling qualifiers.

However, after enabling it for C++ in general I am getting some issues with deduction of templates with pointers that have address spaces in pointees. So I am investigating this further whether it's the right approach at all.

Anastasia marked an inline comment as not done.Nov 27 2018, 12:00 PM
Anastasia updated this revision to Diff 175665.Nov 28 2018, 4:46 AM
  • Changing qualifiers while rebuilding types in TreeTransform is incorrect -> prevent deducing address space instead.
Anastasia marked an inline comment as done.Nov 28 2018, 4:48 AM
Anastasia added inline comments.
lib/Sema/TreeTransform.h
4241 ↗(On Diff #175120)

I think indeed this was wrong! Instead I now just prevent deduction of address spaces for dependent types. I think this is the only use case. So I don't think we need to do anything here any more.

bader added a subscriber: bader.Nov 28 2018, 8:07 AM
bader added inline comments.
lib/Sema/TreeTransform.h
5276 ↗(On Diff #175665)

"cann't" - typo?
cann't - > can't or cannot.

test/CodeGenOpenCLCXX/template-address-spaces.cl
28 ↗(On Diff #175665)

I think it should comment should also be removed.

Anastasia updated this revision to Diff 175698.Nov 28 2018, 8:18 AM

Corrected a typo in comments.

Anastasia updated this revision to Diff 175699.Nov 28 2018, 8:20 AM
Anastasia marked an inline comment as done.

Removed commented code in the test.

Anastasia marked 2 inline comments as done.Nov 28 2018, 8:21 AM
Anastasia added inline comments.
lib/Sema/TreeTransform.h
5276 ↗(On Diff #175665)

Thanks!

test/CodeGenOpenCLCXX/template-address-spaces.cl
28 ↗(On Diff #175665)

Good spot!

rjmccall accepted this revision.Nov 28 2018, 8:38 AM

LGTM.

This revision is now accepted and ready to land.Nov 28 2018, 8:38 AM
This revision was automatically updated to reflect the committed changes.