-
Notifications
You must be signed in to change notification settings - Fork 10.5k
Only emit one copy-to-host or copy-to-accelerator compiler warning per source location, even if it has multiple host uses. #18402
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -602,6 +602,36 @@ void BlocksReachingTensorCode::dump() { | |
// FunctionPartitioner | ||
//===----------------------------------------------------------------------===// | ||
|
||
namespace llvm { | ||
template<typename T> struct DenseMapInfo; | ||
|
||
// An alternative is to SourceManager::getLineAndColumn() as the | ||
// DenseMap/DenseSet key type instead of SourceRange. That would require that | ||
// the call-site translate a SILLocation / SourceRange to line and column | ||
// numbers first. | ||
template<> struct DenseMapInfo<SourceRange> { | ||
static SourceRange getEmptyKey() { return SourceRange(); } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider making the empty key and the tombstone key different? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
||
static SourceRange getTombstoneKey() { | ||
// Make this different from empty key. See for context: | ||
// http://lists.llvm.org/pipermail/llvm-dev/2015-July/088744.html | ||
return SourceRange(SourceLoc( | ||
SMLoc::getFromPointer(DenseMapInfo<const char *>::getTombstoneKey()))); | ||
} | ||
|
||
static unsigned getHashValue(const SourceRange &Val) { | ||
return hash_combine( | ||
DenseMapInfo<const void *>::getHashValue(Val.Start.getOpaquePointerValue()), | ||
DenseMapInfo<const void *>::getHashValue(Val.End.getOpaquePointerValue())); | ||
} | ||
|
||
static bool isEqual(const SourceRange &LHS, | ||
const SourceRange &RHS) { | ||
return LHS == RHS; | ||
} | ||
}; | ||
} // end llvm namespace | ||
|
||
namespace { | ||
/// Marking values in the host program need to either be moved, copied, or have | ||
/// their results sent over to the accelerator. | ||
|
@@ -689,6 +719,11 @@ class TFFunctionPartition { | |
/// Set of all of the __tf_send calls that silence copy-in warnings. | ||
SmallPtrSet<SILInstruction*, 8> explicitCopyMarkers; | ||
|
||
/// Set of source locations where we have issued copy-to-host warnings. | ||
llvm::DenseSet<SourceRange> copyToHostWarningLocs; | ||
/// Set of source locations where we have issued copy-to-accelerator warnings. | ||
llvm::DenseSet<SourceRange> copyToAccelWarningLocs; | ||
|
||
struct PartitionedTensorProgram { | ||
// Initialize all members to NULL. | ||
PartitionedTensorProgram() {} | ||
|
@@ -938,7 +973,10 @@ diagnoseCopyToAccelerator(SILValue value, SILInstruction *user, | |
|
||
auto &ctx = hostFn.getModule().getASTContext(); | ||
|
||
// Emit the warning. | ||
// Emit the warning on this value, if that has not been done before. | ||
if (!copyToAccelWarningLocs.insert(loc.getSourceRange()).second) | ||
return; | ||
|
||
diagnose(ctx, loc.getSourceLoc(), diagID, description) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you need to unique based on the entire source range? Would it be fine to just unique based on loc.getSourceLoc() ? That would eliminate the need for template specialization entirely. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Compiler error for using
|
||
.highlight(loc.getSourceRange()); | ||
auto userLoc = getUserSourceLocation(user); | ||
|
@@ -973,7 +1011,6 @@ diagnoseCopyToAccelerator(SILValue value, SILInstruction *user, | |
/// otherwise emit a warning to tell the programmer that they are doing | ||
/// something that induces an implicit data transfer into their code. | ||
void TFFunctionPartition::diagnoseUsesFromHost(SILValue value, SILLocation loc){ | ||
bool diagnosed = false; | ||
for (auto *use : value->getUses()) { | ||
auto *user = use->getUser(); | ||
|
||
|
@@ -995,8 +1032,8 @@ void TFFunctionPartition::diagnoseUsesFromHost(SILValue value, SILLocation loc){ | |
if (hostFn.getName() == SWIFT_ENTRY_POINT_FUNCTION) | ||
continue; | ||
|
||
// Only emit one warning per use. | ||
if (!diagnosed) | ||
// Only emit one warning per value, even if it has multiple host uses. | ||
if (copyToHostWarningLocs.insert(loc.getSourceRange()).second) | ||
diagnoseCopyToHost(value, user, loc); | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should go in the header that defines SourceRange, and upstreamed :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do!