Enable strict deabstraction in test/integration.swift #18239
Conversation
test/TensorFlow/integration.swift
Outdated
| @@ -49,6 +49,34 @@ public func testTensor(a: Tensor<Float>, b: Tensor<Float>) { | |||
| // CHECK: [[FINISHFN:%.*]] = function_ref @_swift_tfc_FinishTensorComputation | |||
| // CHECK-NEXT: apply [[FINISHFN]]([[PROGRAM]], | |||
|
|
|||
| // STRICTDA-LABEL: --- TFPartition Accelerator Result: {{.*}}testTensor{{.*}} | |||
There was a problem hiding this comment.
it's pretty hard to review these large test diffs -- would it be possible and better if we simply cut over to using strict DA for this test case, and do not generate duplicated test outputs via the STRICTDA prefix?
see https://github.com/apple/swift/pull/18038/files#diff-3ed06a3f2310d2bb22ecdbfc56304e35 as an example, where the diffs are much smaller and easier to verify.
There was a problem hiding this comment.
I can certainly do that (and actually would have saved me quite a bit of work)!
Is there still value in testing the old path where strict deabstraction is disabled?
There was a problem hiding this comment.
Given that we are very close to getting strict deabstraction enabled, we can simply check the strict deabstraction. Done. The PR should be easy to review.
| @@ -239,15 +230,14 @@ public func test_multiple_ifs(status: Bool) { | |||
| _hostOp(a) | |||
| } | |||
|
|
|||
| // The results are different here because compiler optimizes well | |||
There was a problem hiding this comment.
@mhong please make sure this is fine.
There was a problem hiding this comment.
hmm the two if's got merged, probably by the performance pipeline -- if so we'll (temporarily) lose such optimization once we move up the partition pass, and cause another test diff.
So it might be better to rewrite this test with 2 different cond values, like public func test_multiple_ifs(status1: Bool, status2: Bool)
(It could be useful to have separate tests on what the optimizer can do, but this "integration" test suite is not the best place for that.)
There was a problem hiding this comment.
Done. I just changed the second condition instead of adding a new variable.
| @@ -503,17 +480,17 @@ public func testNoInlineUser() { | |||
| noInlineUser(x) | |||
| } | |||
|
|
|||
| // NOTE: the output for STRICT_DA does not have strong_retain or release. | |||
There was a problem hiding this comment.
@mhong Please make sure this is also OK.
| @@ -503,17 +480,17 @@ public func testNoInlineUser() { | |||
| noInlineUser(x) | |||
| } | |||
|
|
|||
| // NOTE: the output for STRICT_DA does not have strong_retain or release. | |||
test/TensorFlow/integration.swift
Outdated
| // CHECK-NEXT: %5 = metatype $@thick Float.Type | ||
| // CHECK-NEXT: %6 = string_literal utf8 "/device:CPU:0" | ||
| // CHECK-NEXT: %7 = builtin "__tfop_Sub,$in,$in,T,__device"(%4 : $TensorHandle<Float>, %4 : $TensorHandle<Float>, %5 : $@thick Float.Type, %6 : $Builtin.RawPointer) : $TensorHandle<Float> | ||
| // CHECK-NEXT: [[A:%.*]] = graph_op "Add,i,i"(%0 : $TensorHandle<Float>, %0 : $TensorHandle<Float>) |
There was a problem hiding this comment.
can we add back the checks on attrs like T and __device? Doing it on the first graph_op inst (or some of the insts) should be sufficient.
one example is:
%2 = graph_op "Add,i,i"(%0 : $TensorHandle<Float>, %0 : $TensorHandle<Float>) {T: $Float, __device: "/device:CPU:0"} : $TensorHandle<Float> // users: %3, %3
| @@ -239,15 +230,14 @@ public func test_multiple_ifs(status: Bool) { | |||
| _hostOp(a) | |||
| } | |||
|
|
|||
| // The results are different here because compiler optimizes well | |||
There was a problem hiding this comment.
hmm the two if's got merged, probably by the performance pipeline -- if so we'll (temporarily) lose such optimization once we move up the partition pass, and cause another test diff.
So it might be better to rewrite this test with 2 different cond values, like public func test_multiple_ifs(status1: Bool, status2: Bool)
(It could be useful to have separate tests on what the optimizer can do, but this "integration" test suite is not the best place for that.)
Partially resolves SR-8304.