Implement lowergraph support for string/int/float/bool arrays #18041
Conversation
Fix SR11079/SR11079. ShapeList and type lists remain.
| @swift-ci please test tensorflow |
generating diagnostic errors if the element types mismatch the dtype instead of crashing the compiler.
| @swift-ci please test tensorflow |
2 similar comments
| @swift-ci please test tensorflow |
| @swift-ci please test tensorflow |
| | ||
| | ||
| public func testConvolution(x: Tensor<Float>, filter: Tensor<Float>) -> Tensor<Float> { | ||
| // expected-error @+1 {{FIXME: Handle array attributes}} |
| | ||
| // TODO: TF_SetAttrTypeList | ||
| | ||
| if (elementTypeString == "String") { |
There was a problem hiding this comment.
is there any enum that we can match on, rather than doing (sub)string comparison?
There was a problem hiding this comment.
Unfortunately, not really (that I know of). This really comes down to what types we want to allow in #tfop attribute lists. We could tighten this up to check that the types are coming from Swift module. I'll dig around to see what ASTContext has to work with here.
| | ||
| if (elementTypeString == "String") { | ||
| SmallVector<const void*, 4> pointers; | ||
| SmallVector<size_t, 4> sizes; |
There was a problem hiding this comment.
do we want to call reserve() for pointers and sizes as well? same for values below.
| SmallVector<float, 4> values; | ||
| for (auto elt : elements) { | ||
| auto value = elt.getFloatValue(); | ||
| bool losesInfo = false; |
There was a problem hiding this comment.
add a comment on why losing precision here is ok?
There was a problem hiding this comment.
well it really isn't, but this is tensorflow.... :-)
There was a problem hiding this comment.
(but sure, I'll add a comment)
| for (auto elt : attrValue.getArrayValue(eltType)) | ||
| addScalar(elt); | ||
| for (auto elt : attrValue.getArrayValue(eltType)) { | ||
| if (addScalar(elt)) |
There was a problem hiding this comment.
minor: i'd prefer that we pass elements explicitly into addScalar call here, so that the code here looks less "magical". Thoughts?
| elements.push_back(floatValue.bitcastToAPInt()); | ||
| } | ||
| | ||
| if (elements.back().getBitWidth() != dtypeSize*8) { |
There was a problem hiding this comment.
nit: dtypeSize * 8 instead of dtypeSize*8?
| // The scalar case is very simple, the shape of a scalar is 0d, and the | ||
| // data type comes from an attr that should already be processed. | ||
| auto addScalar = [&](SymbolicValue value) { | ||
| // Add a scalar to the elements list, checking that it is the right size |
There was a problem hiding this comment.
"checking that it is the right size" -> should we update the comment as we are actually fixing the sizes in the impl?
| Thanks for the feedback! |
| @swift-ci please test tensorflow |
5 similar comments
| @swift-ci please test tensorflow |
| @swift-ci please test tensorflow |
| @swift-ci please test tensorflow |
| @swift-ci please test tensorflow |
| @swift-ci please test tensorflow |
| sigh. :) |
| @swift-ci please test tensorflow |
5 similar comments
| @swift-ci please test tensorflow |
| @swift-ci please test tensorflow |
| @swift-ci please test tensorflow |
| @swift-ci please test tensorflow |
| @swift-ci please test tensorflow |
| Looks like CI is busy with a time-driven run: https://ci-external.swift.org/computer/ubuntu-16.04-tensorflow/ |
| Weird how it doesn't provide any feedback... |
| @swift-ci please test tensorflow |
| @swift-ci please test tensorflow |
| it's not always listening |
| It's running now: https://ci-external.swift.org/job/swift-PR-TensorFlow-Linux/. But sometimes this PR page still wouldn't show that, so I usually check the above CI link. |
| @swift-ci please test tensorflow |
| resolved merge conflict, trying again. |
| @swift-ci please test tensorflow |
| auto value = elt.getFloatValue(); | ||
| // TensorFlow only supports float32 attributes, and isn't designed | ||
| // for high precision, so we force truncate down. | ||
| bool losesInfo = false; |
There was a problem hiding this comment.
nit: Some of this could be refactored into helper functions. I guess we can do it later.
Fixes SR11079/SR11079.
ShapeList and type lists remain.