This is an archive of the discontinued LLVM Phabricator instance.

Differential D87230

[MLIR] Fix Win test due to partial order of CHECK directives
ClosedPublic

Authored by nicolasvasilache on Sep 7 2020, 3:06 AM.

Details

Reviewers
ftynse
aartbik
Commits
rG1c849ec40a53: [MLIR] Fix Win test due to partial order of CHECK directives

Diff Detail

Event Timeline

nicolasvasilache created this revision.Sep 7 2020, 3:06 AM
Herald added a reviewer: aartbik. · View Herald TranscriptSep 7 2020, 3:06 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: msifontes, jurahul, Kayjukh and 12 others. · View Herald Transcript
nicolasvasilache requested review of this revision.Sep 7 2020, 3:06 AM
Herald added a subscriber: stephenneuendorffer. · View Herald TranscriptSep 7 2020, 3:06 AM
nicolasvasilache updated this revision to Diff 290231.Sep 7 2020, 3:09 AM

Update.

Harbormaster completed remote builds in B70811: Diff 290231.Sep 7 2020, 3:29 AM
Harbormaster completed remote builds in B70810: Diff 290230.Sep 7 2020, 3:41 AM
ftynse accepted this revision.Sep 7 2020, 3:43 AM
This revision is now accepted and ready to land.Sep 7 2020, 3:43 AM
Closed by commit rG1c849ec40a53: [MLIR] Fix Win test due to partial order of CHECK directives (authored by nicolasvasilache). · Explain WhySep 7 2020, 5:15 AM
This revision was automatically updated to reflect the committed changes.
nicolasvasilache added a commit: rG1c849ec40a53: [MLIR] Fix Win test due to partial order of CHECK directives.
rriddle added inline comments.Sep 7 2020, 1:57 PM
mlir/test/Conversion/VectorToSCF/vector-to-loops.mlir
102

What is the cause of this? This revision looks like it's working around a bug in the implementation that relies on something that is non-deterministic w.r.t toolchain.

nicolasvasilache marked an inline comment as done.Sep 7 2020, 2:32 PM
nicolasvasilache added inline comments.
mlir/test/Conversion/VectorToSCF/vector-to-loops.mlir
102

There is a:

Value vector = vector_insert_element(remote(indicesRef), local(ivsRef.drop_back()), pos);

here: https://github.com/llvm/llvm-project/blob/master/mlir/lib/Conversion/VectorToSCF/VectorToSCF.cpp#L600

As discussed in: https://llvm.discourse.group/t/evolving-builder-apis-based-on-lessons-learned-from-edsc/879.

mehdi_amini added inline comments.Sep 7 2020, 3:37 PM
mlir/test/Conversion/VectorToSCF/vector-to-loops.mlir
102

Instead of masking the issue, can you replace the uses of EDSCs with builders in this code?

nicolasvasilache marked an inline comment as done.Sep 8 2020, 12:16 AM
nicolasvasilache added subscribers: burmako, dmgreen, fhahn and 4 others.

First this is not an issue but a feature of C++.
Builders would have the same behavior as explained in the discourse post.

mehdi_amini added a subscriber: nicolasvasilache.Sep 8 2020, 12:20 AM

Can you then split the statement to have deterministic portable behavior?

nicolasvasilache added a comment.Sep 8 2020, 12:40 AM

yes :) https://reviews.llvm.org/D87273

Revision Contents

PathSize
mlir/
test/
Conversion/
VectorToSCF/
8 lines

Diff 290253

mlir/test/Conversion/VectorToSCF/vector-to-loops.mlir

Show First 20 Lines • Show All 93 Lines • ▼ Show 20 Linesfunc @materialize_read(%M: index, %N: index, %O: index, %P: index) {
// CHECK-NEXT: {{.*}} = affine.apply #[[$ADD]](%[[I3]], %[[I6]]) // CHECK-NEXT: {{.*}} = affine.apply #[[$ADD]](%[[I3]], %[[I6]])
// CHECK-NEXT: {{.*}} = affine.apply #[[$SUB]]()[%{{.*}}] // CHECK-NEXT: {{.*}} = affine.apply #[[$SUB]]()[%{{.*}}]
// CHECK-NEXT: {{.*}} = cmpi "slt", {{.*}} : index // CHECK-NEXT: {{.*}} = cmpi "slt", {{.*}} : index
// CHECK-NEXT: {{.*}} = select // CHECK-NEXT: {{.*}} = select
// CHECK-NEXT: {{.*}} = cmpi "slt", {{.*}}, %[[C0]] : index // CHECK-NEXT: {{.*}} = cmpi "slt", {{.*}}, %[[C0]] : index
// CHECK-NEXT: %[[L3:.*]] = select // CHECK-NEXT: %[[L3:.*]] = select
// CHECK-NEXT: %[[VIDX:.*]] = index_cast %[[I4]] // CHECK-NEXT: %[[VIDX:.*]] = index_cast %[[I4]]
// //
// CHECK-NEXT: {{.*}} = load %{{.*}}[%[[L0]], %[[L1]], %[[L2]], %[[L3]]] : memref<?x?x?x?xf32> // CHECK-DAG: %[[SCAL:.*]] = load %{{.*}}[%[[L0]], %[[L1]], %[[L2]], %[[L3]]] : memref<?x?x?x?xf32>
rriddleUnsubmitted
Done
ReplyInline Actions

What is the cause of this? This revision looks like it's working around a bug in the implementation that relies on something that is non-deterministic w.r.t toolchain.

rriddle: What is the cause of this? This revision looks like it's working around a bug in the…
nicolasvasilacheAuthorUnsubmitted
Done
ReplyInline Actions

There is a:

Value vector = vector_insert_element(remote(indicesRef), local(ivsRef.drop_back()), pos);

here: https://github.com/llvm/llvm-project/blob/master/mlir/lib/Conversion/VectorToSCF/VectorToSCF.cpp#L600

As discussed in: https://llvm.discourse.group/t/evolving-builder-apis-based-on-lessons-learned-from-edsc/879.

nicolasvasilache: There is a: ``` Value vector = vector_insert_element(remote(indicesRef), local(ivsRef.
mehdi_aminiUnsubmitted
Not Done
ReplyInline Actions

Instead of masking the issue, can you replace the uses of EDSCs with builders in this code?

mehdi_amini: Instead of masking the issue, can you replace the uses of EDSCs with builders in this code?
// CHECK-NEXT: %[[VEC:.*]] = load %[[ALLOC]][%[[I6]], %[[I5]]] : memref<5x4xvector<3xf32>> // CHECK-DAG: %[[VEC:.*]] = load %[[ALLOC]][%[[I6]], %[[I5]]] : memref<5x4xvector<3xf32>>
// CHECK-NEXT: %[[RVEC:.*]] = vector.insertelement %25, %[[VEC]][%[[VIDX]] : i32] : vector<3xf32> // CHECK-NEXT: %[[RVEC:.*]] = vector.insertelement %[[SCAL]], %[[VEC]][%[[VIDX]] : i32] : vector<3xf32>
// CHECK-NEXT: store %[[RVEC]], %[[ALLOC]][%[[I6]], %[[I5]]] : memref<5x4xvector<3xf32>> // CHECK-NEXT: store %[[RVEC]], %[[ALLOC]][%[[I6]], %[[I5]]] : memref<5x4xvector<3xf32>>
// CHECK-NEXT: } // CHECK-NEXT: }
// CHECK-NEXT: } // CHECK-NEXT: }
// CHECK-NEXT: } // CHECK-NEXT: }
// CHECK-NEXT: } // CHECK-NEXT: }
// CHECK-NEXT: } // CHECK-NEXT: }
// CHECK-NEXT: } // CHECK-NEXT: }
// CHECK-NEXT: } // CHECK-NEXT: }
▲ Show 20 Lines • Show All 65 Lines • ▼ Show 20 Linesfunc @materialize_write(%M: index, %N: index, %O: index, %P: index) {
// CHECK-NEXT: {{.*}} = cmpi "slt", {{.*}}, {{.*}} : index // CHECK-NEXT: {{.*}} = cmpi "slt", {{.*}}, {{.*}} : index
// CHECK-NEXT: {{.*}} = select {{.*}}, {{.*}}, {{.*}} : index // CHECK-NEXT: {{.*}} = select {{.*}}, {{.*}}, {{.*}} : index
// CHECK-NEXT: {{.*}} = cmpi "slt", {{.*}}, %[[C0]] : index // CHECK-NEXT: {{.*}} = cmpi "slt", {{.*}}, %[[C0]] : index
// CHECK-NEXT: %[[S3:.*]] = select {{.*}}, %[[C0]], {{.*}} : index // CHECK-NEXT: %[[S3:.*]] = select {{.*}}, %[[C0]], {{.*}} : index
// CHECK-NEXT: %[[VIDX:.*]] = index_cast %[[I4]] // CHECK-NEXT: %[[VIDX:.*]] = index_cast %[[I4]]
// //
// CHECK-NEXT: %[[VEC:.*]] = load {{.*}}[%[[I6]], %[[I5]]] : memref<5x4xvector<3xf32>> // CHECK-NEXT: %[[VEC:.*]] = load {{.*}}[%[[I6]], %[[I5]]] : memref<5x4xvector<3xf32>>
// CHECK-NEXT: %[[SCAL:.*]] = vector.extractelement %[[VEC]][%[[VIDX]] : i32] : vector<3xf32> // CHECK-NEXT: %[[SCAL:.*]] = vector.extractelement %[[VEC]][%[[VIDX]] : i32] : vector<3xf32>
// CHECK: store %[[SCAL]], {{.*}}[%[[S0]], %[[S1]], %[[S2]], %[[S3]]] : memref<?x?x?x?xf32> // CHECK-NEXT: store %[[SCAL]], {{.*}}[%[[S0]], %[[S1]], %[[S2]], %[[S3]]] : memref<?x?x?x?xf32>
// CHECK-NEXT: } // CHECK-NEXT: }
// CHECK-NEXT: } // CHECK-NEXT: }
// CHECK-NEXT: } // CHECK-NEXT: }
// CHECK-NEXT: } // CHECK-NEXT: }
// CHECK-NEXT: } // CHECK-NEXT: }
// CHECK-NEXT: } // CHECK-NEXT: }
// CHECK-NEXT: } // CHECK-NEXT: }
// CHECK-NEXT: return // CHECK-NEXT: return
▲ Show 20 Lines • Show All 293 Lines • Show Last 20 Lines