Skip to content
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

[MLIR][XLA] Add GatherOp to LHLO/HLO emitters #40578

Conversation

xinan-jiang
Copy link
Contributor

This is a PR from JIZHI, the AI platform in Tencent.
@sherhut @pifon2a

We work on TensorFlow/MLIR to make mlir_gpu enable. Could you tell me how to run the test? It seems need a new tool.

@google-ml-butler google-ml-butler bot added the size:M CL Change Size: Medium label Jun 18, 2020
@gbaned gbaned self-assigned this Jun 18, 2020
@joker-eph
Copy link
Contributor

@timshen91 can you help providing guidance about where we want the HLO->LHLO conversion be handled right now?

Copy link
Contributor

@pifon2a pifon2a left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you so much! Could you also add xla_hlo.GatherOp -> xla_lhlo.GatherOp conversion to hlo_legalize_to_lhlo.cc? it should be just one line to populate the patterns, one line in map_hlo_to_lhlo_op.h to map the ops and a test.


// CHECK: func @gather(%[[ARG0:.*]]: [[TYPE0:.*]], %[[ARG1:.*]]: [[TYPE1:.*]], %[[RESULT:.*]]: [[RTYPE:.*]]) {
// CHECK: "xla_lhlo.gather"(%[[ARG0]], %[[ARG1]], %[[RESULT]])
// CHECK: {collapsed_slice_dims = dense<0> : tensor<1xi64>, index_vector_dim = 2 : i64, offset_dims = dense<2> : tensor<1xi64>, slice_sizes = dense<[1, 10]> : tensor<2xi64>, start_index_map = dense<0> : tensor<1xi64>} : ([[TYPE0]], [[TYPE1]], [[RTYPE]]) -> ()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: please use CHECK-SAME to wrap the line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor

@pifon2a pifon2a left a comment

Choose a reason for hiding this comment

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

git clang-format?

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Jun 19, 2020
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jun 19, 2020
input.offset_dims().end());
std::vector<int64> gather_collapsed_slice_dims(
input.collapsed_slice_dims().begin(), input.collapsed_slice_dims().end());
std::vector<int64> gather_start_index_map(input.start_index_map().begin(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: use SmallVector instead. I assume that these attributes aren't very large in general.
(see LhloDialectEmitter::HandleReduceWindow for example)

@joker-eph joker-eph requested a review from timshen91 June 19, 2020 23:49
@rthadur rthadur removed the ready to pull PR ready for merge process label Jun 20, 2020
@gbaned
Copy link
Contributor

gbaned commented Jun 22, 2020

@xinan-jiang Can you please check @joker-eph's comments and keep us posted. Thanks!

@gbaned gbaned added the stat:awaiting response Status - Awaiting response from author label Jun 22, 2020
@timshen91 timshen91 removed their request for review June 22, 2020 20:20
@timshen91
Copy link
Member

Unassigning myself since @pifon2a is more familiar with this code.

@pifon2a pifon2a added the ready to pull PR ready for merge process label Jun 25, 2020
@tensorflow-copybara tensorflow-copybara merged commit 93e1247 into tensorflow:master Jun 26, 2020
@gbaned gbaned removed the stat:awaiting response Status - Awaiting response from author label Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes ready to pull PR ready for merge process size:M CL Change Size: Medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants