Skip to content

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

Merged
merged 2 commits into from
Jul 31, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 41 additions & 4 deletions lib/SILOptimizer/Mandatory/TFPartition.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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> {
Copy link
Contributor

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 :-)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do!

static SourceRange getEmptyKey() { return SourceRange(); }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider making the empty key and the tombstone key different?

Copy link
Author

Choose a reason for hiding this comment

The 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.
Expand Down Expand Up @@ -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() {}
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DenseMapInfo is not defined for SourceLoc either, so I might as make SourceRange work as the key type.

Compiler error for using llvm::DenseSet<SourceLoc> copyToHostWarningLocs:

/usr/local/google/home/hongm/ssd_part/git/swift-base/llvm/include/llvm/ADT/DenseMap.h:421:22: error: no member named 'getEmptyKey' in 'llvm::DenseMapInfo<swift::SourceLoc>'
    return KeyInfoT::getEmptyKey();

.highlight(loc.getSourceRange());
auto userLoc = getUserSourceLocation(user);
Expand Down Expand Up @@ -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();

Expand All @@ -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);
}
}
Expand Down
1 change: 0 additions & 1 deletion test/TensorFlow/crashers.swift
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,6 @@ public func testMultiResultOp_send_recv() {
// Accelerator -> Host
_hostOp(x)
x += [[2.0]]
// expected-warning @+2{{implicitly copied to the host}}
// expected-warning @+1{{implicitly copied to the host}}
let results = TensorFlow.Raw.softmaxCrossEntropyWithLogits(features: x, labels: x)
// Accelerator -> Host
Expand Down
20 changes: 20 additions & 0 deletions test/TensorFlow/diagnostics.swift
Original file line number Diff line number Diff line change
Expand Up @@ -46,3 +46,23 @@ public func testDevice() {
_hostOp(extra)
}

// This loop is unrolled, so we have multiple SIL values for `x`, but there
// should be a single copy-to-host compiler warning.
public func SR8412_CopyToHost() {
for _ in 0...10 {
let x = Tensor(1) // expected-warning {{value implicitly copied to the host}}
_hostOp(x)
}
}

@inline(never)
public func hostFunc() -> Tensor<Float> {
return Tensor<Float>(1.0)
}

public func SR8412_CopyToAccel(a: Int32) {
let x = Tensor<Float>(1.0)
for _ in 0...10 {
let _ = hostFunc() + x // expected-warning {{value implicitly copied to the accelerator}}
}
}
4 changes: 2 additions & 2 deletions test/TensorFlow/integration.swift
Original file line number Diff line number Diff line change
Expand Up @@ -458,8 +458,8 @@ public struct NonInlineMethodExample {
var b = Tensor<Float>(2.0)

@inline(never)
public mutating func mutatingMethod() { // expected-warning 2 {{value implicitly copied}}
a += b // expected-note 2 {{value used here}}
public mutating func mutatingMethod() { // expected-warning {{value implicitly copied}}
a += b // expected-note {{value used here}}
b += a
}
}
Expand Down