This is an archive of the discontinued LLVM Phabricator instance.

[CUDA] When we emit an error that might have been deferred, also print a callstack.
ClosedPublic

Authored by jlebar on Oct 17 2016, 3:03 PM.

Details

Summary

Previously, when you did something not allowed in a host+device function
and then caused it to be codegen'ed, we would print out an error telling
you that you did something bad, but we wouldn't tell you how we decided
that the function needed to be codegen'ed.

This change causes us to print out a callstack when emitting deferred
errors. This is immensely helpful when debugging highly-templated code,
where it's often unclear how a function became known-emitted.

We only print the callstack once per function, after we print the all
deferred errors.

This patch also switches all of our hashtables to using canonical
FunctionDecls instead of regular FunctionDecls. This prevents a number
of bugs, some of which are caught by tests added here, in which we
assume that two FDs for the same function have the same pointer value.

Diff Detail

Repository
rL LLVM

Event Timeline

jlebar updated this revision to Diff 74912.Oct 17 2016, 3:03 PM
jlebar retitled this revision from to [CUDA] When we emit an error that might have been deferred, also print a callstack..
jlebar updated this object.
jlebar added a reviewer: rnk.
jlebar added subscribers: tra, cfe-commits.
rnk added inline comments.Oct 19 2016, 9:36 AM
clang/include/clang/Sema/Sema.h
9292 ↗(On Diff #74912)

Rather than having a custom key type, maybe it would be better to phrase this as a MapVector<CanonicalDeclPtr<FunctionDecl>, SourceLocation> ? Despite all the comments, I assumed FunctionDeclAndLoc was hashed by both elements for a long time.

9322 ↗(On Diff #74912)

typo on "immediate"

jlebar updated this revision to Diff 75178.Oct 19 2016, 11:20 AM
jlebar marked 2 inline comments as done.

Address rnk's comments.

clang/include/clang/Sema/Sema.h
9292 ↗(On Diff #74912)

That's much better; thank you.

rnk accepted this revision.Oct 19 2016, 11:42 AM
rnk edited edge metadata.

lgtm

clang/include/clang/Basic/DiagnosticSemaKinds.td
6707 ↗(On Diff #75178)

Do you think it's worth trying to indicate why the root of the "called by" notes must be emitted? I'm not suggesting we do it in this patch, just wondering.

clang/include/clang/Sema/Sema.h
9259 ↗(On Diff #75178)

So, part of me wants to use FunctionDeclAndLoc here instead of std::pair, but then you'd have to bring back all the hashing machinery instead of getting it for free. Up to you, I guess.

9274 ↗(On Diff #75178)

I guess there can be many callers, but we always record the first one that caused this function to be emitted.

This revision is now accepted and ready to land.Oct 19 2016, 11:42 AM
jlebar marked 2 inline comments as done.Oct 19 2016, 2:10 PM

I'm going to submit this and send a patch to reuse FunctionDeclAndLoc. But I'm happy to add a comment about the note as well.

clang/include/clang/Basic/DiagnosticSemaKinds.td
6707 ↗(On Diff #75178)

It seems just like e.g. note_template_class_instantiation_here, so I am not entirely sure what is the point of confusion. But if you can clarify that, I am happy to add a comment in a separate patch -- this stuff is already quite complicated, so I'm in favor of whatever we can do to make it clearer.

clang/include/clang/Sema/Sema.h
9259 ↗(On Diff #75178)

Oh, and now make its hash function depend on both members instead of just the FD?

I actually like that change, but let me make it in a separate patch.

9274 ↗(On Diff #75178)

Yes, exactly.

I was briefly worried about cycles here, but I think we're OK because ultimately we need to end up at an a priori known-emitted function, and that's a root in this tree.

jlebar marked 2 inline comments as done.Oct 19 2016, 2:10 PM

Thank you for the review, Reid.

This revision was automatically updated to reflect the committed changes.