Fix onnx.GatherND and onnx.ScatterND issues with dynamic indices #2550

negiyas · 2023-10-04T06:29:46Z

This PR fixes the following onnx.GatherND and onnx.ScatterND issues with dynamic indices.
With the following two lit tests, latest onnx-mlir causes internal errors while onnx-to-kernel conversion, this PR fixes them.

TODOs
[ X] Enable backend tests

// RUN: onnx-mlir-opt --shape-inference --convert-onnx-to-krnl %s -split-input-file | FileCheck %s

// COM: Test GatherND with dynamic shape
func.func @test_gather_nd_with_dynamic_shape(%arg0 : tensor<2x2xf32>, %arg1 : tensor<?x2xi64>) -> tensor<?xf32> {
  %0 = "onnx.GatherND"(%arg0, %arg1) {batch_dims = 0 : si64} : (tensor<2x2xf32>, tensor<?x2xi64>) -> tensor<?xf32>
  "func.return"(%0) : (tensor<?xf32>) -> ()
}

// COM: Test GatherND with dynamic shape
func.func @test_scatter_nd_with_dynamic_indices(%arg0: tensor<2x1xi64>, %arg1: tensor<?x2xi64>, %arg2: tensor<2xi64>) -> tensor<2x1xi64> {
  %0 = "onnx.ScatterND"(%arg0, %arg1, %arg2) {reduction = "none"} : (tensor<2x1xi64>, tensor<?x2xi64>, tensor<2xi64>) -> tensor<2x1xi64>]
  return %0 : tensor<2x1xi64>
}

…ynamic indices. Signed-off-by: Yasushi Negishi <[email protected]>

Signed-off-by: Yasushi Negishi <[email protected]>

AlexandreEichenberger

Is there also a backend test that we could activate to test this feature through execution of an operation?

src/Conversion/ONNXToKrnl/Tensor/ScatterND.cpp

negiyas · 2023-10-11T00:45:58Z

Is there also a backend test that we could activate to test this feature through execution of an operation?
Thanks for the comments. I am preparing a bachend test.

Signed-off-by: Yasushi Negishi <[email protected]>

…d dynamica cases. Signed-off-by: Yasushi Negishi <[email protected]>

Signed-off-by: Yasushi Negishi <[email protected]>

… indices. Signed-off-by: Yasushi Negishi <[email protected]>

Signed-off-by: Yasushi Negishi <[email protected]>

negiyas · 2023-10-17T05:57:17Z

Is there also a backend test that we could activate to test this feature through execution of an operation?
Thanks for the comments. I am preparing a bachend test.

I added backend tests for onnx.GatherND with dynamic indices cases. Thanks.

AlexandreEichenberger · 2023-10-24T23:50:26Z

src/Conversion/ONNXToKrnl/Tensor/GatherND.cpp

+    IndexExpr indicesDimsSize = oneIE;
+    for (int64_t i = 0; i < indicesRank; i++)
+      indicesDimsSize = indicesDimsSize * indicesDims[i];
+    IndexExpr BDS(batchDimsSize),


Quick question: is this reshape valid even when indicesDimSizes is not divisible by batchDimSize * indicesLastDim ? Or is it guaranteed? Maybe you can write a comment explaining why this works here in the code.

@AlexandreEichenberger Thanks for the comments.

Alex> Quick question: is this reshape valid even when indicesDimSizes is not divisible by batchDimSize * indicesLastDim ?
Alex> Or is it guaranteed? Maybe you can write a comment explaining why this works here in the code.

It is guaranteed, because IDS can be calculated by product of a part of the indices dimensions.
I changed the code to calculate IDS by using "product" instead of "floorDiv" to clarify the meaning.

… calculate new dimensions. Signed-off-by: Yasushi Negishi <[email protected]>

AlexandreEichenberger

LGTM, thanks much clearer without the DIV operation

Signed-off-by: Yasushi Negishi <[email protected]>

jenkins-droid · 2023-10-26T04:02:07Z

Jenkins Linux s390x Build #13132 [push] Fix onnx.GatherND and on... started at 00:02

jenkins-droid · 2023-10-26T04:02:11Z

Jenkins Linux ppc64le Build #12125 [push] Fix onnx.GatherND and on... started at 00:09

jenkins-droid · 2023-10-26T04:02:16Z

Jenkins Linux amd64 Build #13108 [push] Fix onnx.GatherND and on... started at 23:02

jenkins-droid · 2023-10-26T05:19:41Z

Jenkins Linux amd64 Build #13108 [push] Fix onnx.GatherND and on... passed after 1 hr 17 min

jenkins-droid · 2023-10-26T05:29:28Z

Jenkins Linux s390x Build #13132 [push] Fix onnx.GatherND and on... passed after 1 hr 27 min

jenkins-droid · 2023-10-26T05:38:59Z

Jenkins Linux ppc64le Build #12125 [push] Fix onnx.GatherND and on... passed after 1 hr 36 min

negiyas added 2 commits October 4, 2023 02:19

Add test for onnx.GatherND, onnx.ScatterND and onnx.Gather ops with d…

c56b620

…ynamic indices. Signed-off-by: Yasushi Negishi <[email protected]>

Merge branch 'main' into issue_gather_scatter_nd_with_dynamic_indices

5925bfe

negiyas marked this pull request as draft October 4, 2023 06:44

Fix comments on the lit test

943082e

Signed-off-by: Yasushi Negishi <[email protected]>

negiyas mentioned this pull request Oct 5, 2023

Issues with internal errors while onnx-to-kernel conversion for onnx.GatherND and onnx.ScatterND with dynamic indices. #2552

Open

negiyas added 3 commits October 5, 2023 08:11

Fix a typo in lit ltest.

decbfb5

Signed-off-by: Yasushi Negishi <[email protected]>

Fix issues for onnx.GatherND and onnx.ScatterND with dynamic indices.

37867ee

Signed-off-by: Yasushi Negishi <[email protected]>

Merge branch 'main' into issue_gather_scatter_nd_with_dynamic_indices

d948e11

negiyas changed the title ~~[WIP] Lit tests to check onnx.GatherND and onnx.ScatterND issues with dynamic indices~~ [WIP] Fix onnx.GatherND and onnx.ScatterND issues with dynamic indices Oct 10, 2023

negiyas added 3 commits October 10, 2023 01:01

Fix clang-format issues.

245deea

Signed-off-by: Yasushi Negishi <[email protected]>

Fix a compilation error.

8592a35

Signed-off-by: Yasushi Negishi <[email protected]>

Fix clang-format issues.

327121a

Signed-off-by: Yasushi Negishi <[email protected]>

negiyas changed the title ~~[WIP] Fix onnx.GatherND and onnx.ScatterND issues with dynamic indices~~ Fix onnx.GatherND and onnx.ScatterND issues with dynamic indices Oct 10, 2023

negiyas marked this pull request as ready for review October 10, 2023 04:22

AlexandreEichenberger reviewed Oct 10, 2023

View reviewed changes

src/Conversion/ONNXToKrnl/Tensor/ScatterND.cpp Outdated Show resolved Hide resolved

negiyas added 12 commits October 11, 2023 00:52

use std::min instead of #define MIN in ScatterND.cpp

5e25925

Signed-off-by: Yasushi Negishi <[email protected]>

Add dynamic cases in backend tests for scatternd/gathernd

441981a

Signed-off-by: Yasushi Negishi <[email protected]>

Fixed issues with "test_gathernd_example_int32_cpu" in both static an…

f957c49

…d dynamica cases. Signed-off-by: Yasushi Negishi <[email protected]>

Fix backend test issues for onnx.gatherND.

ce76f99

Signed-off-by: Yasushi Negishi <[email protected]>

Merge branch 'main' into issue_gather_scatter_nd_with_dynamic_indices

22bf4e1

Fixing lit and backend tests for onnx.GatherND.

259722c

Signed-off-by: Yasushi Negishi <[email protected]>

Merge branch 'main' into issue_gather_scatter_nd_with_dynamic_indices

51d036c

Fix backend test for onnx.GatherND with dynamic indices.

5863ccf

Signed-off-by: Yasushi Negishi <[email protected]>

Fix bakend tests for onnx.gatherND with dynamic indices.

b447895

Signed-off-by: Yasushi Negishi <[email protected]>

Fix lit tests and backend tests errors for onnx.GatherND with dynamic…

36b4c90

… indices. Signed-off-by: Yasushi Negishi <[email protected]>

Merge branch 'main' into issue_gather_scatter_nd_with_dynamic_indices

112c7e7

Fix python format of test/backend/inference_backend.py

8ff9cc5

Signed-off-by: Yasushi Negishi <[email protected]>

Merge branch 'main' into issue_gather_scatter_nd_with_dynamic_indices

e945fdb

Merge branch 'main' into issue_gather_scatter_nd_with_dynamic_indices

21f0325

negiyas added the Ready for Review label Oct 20, 2023

negiyas added 2 commits October 23, 2023 01:01

Merge branch 'main' into issue_gather_scatter_nd_with_dynamic_indices

9dbfa26

Merge branch 'main' into issue_gather_scatter_nd_with_dynamic_indices

f0c56b1

AlexandreEichenberger reviewed Oct 24, 2023

View reviewed changes

negiyas added 2 commits October 25, 2023 08:30

Merge branch 'main' into issue_gather_scatter_nd_with_dynamic_indices

c54d323

Change src/Conversion/ONNXToKrnl/Tensor/GatherND.cpp to change how to…

42c2882

… calculate new dimensions. Signed-off-by: Yasushi Negishi <[email protected]>

AlexandreEichenberger approved these changes Oct 25, 2023

View reviewed changes

negiyas added 2 commits October 26, 2023 00:28

Merge branch 'main' into issue_gather_scatter_nd_with_dynamic_indices

cf837be

Merge branch 'main' into issue_gather_scatter_nd_with_dynamic_indices

ffb0adf

Signed-off-by: Yasushi Negishi <[email protected]>

negiyas merged commit ee36a16 into onnx:main Oct 26, 2023
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix onnx.GatherND and onnx.ScatterND issues with dynamic indices #2550

Fix onnx.GatherND and onnx.ScatterND issues with dynamic indices #2550

negiyas commented Oct 4, 2023 •

edited

Loading

AlexandreEichenberger left a comment

negiyas commented Oct 11, 2023

negiyas commented Oct 17, 2023

AlexandreEichenberger Oct 24, 2023

negiyas Oct 25, 2023

AlexandreEichenberger left a comment

jenkins-droid commented Oct 26, 2023

jenkins-droid commented Oct 26, 2023

jenkins-droid commented Oct 26, 2023

jenkins-droid commented Oct 26, 2023

jenkins-droid commented Oct 26, 2023

jenkins-droid commented Oct 26, 2023

Fix onnx.GatherND and onnx.ScatterND issues with dynamic indices #2550

Fix onnx.GatherND and onnx.ScatterND issues with dynamic indices #2550

Conversation

negiyas commented Oct 4, 2023 • edited Loading

AlexandreEichenberger left a comment

Choose a reason for hiding this comment

negiyas commented Oct 11, 2023

negiyas commented Oct 17, 2023

AlexandreEichenberger Oct 24, 2023

Choose a reason for hiding this comment

negiyas Oct 25, 2023

Choose a reason for hiding this comment

AlexandreEichenberger left a comment

Choose a reason for hiding this comment

jenkins-droid commented Oct 26, 2023

jenkins-droid commented Oct 26, 2023

jenkins-droid commented Oct 26, 2023

jenkins-droid commented Oct 26, 2023

jenkins-droid commented Oct 26, 2023

jenkins-droid commented Oct 26, 2023

negiyas commented Oct 4, 2023 •

edited

Loading