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

[Tosa] Add legalization of BroadcastTo #60692

Merged
merged 1 commit into from
Jul 24, 2023

Conversation

Tai78641
Copy link
Contributor

@Tai78641 Tai78641 commented May 24, 2023

For tf/tfl boradcast-to operator with input and shape where:

  • shape is compile time constant, and
  • shape's rank is greater than or equal to input's rank, and
  • input element type is not complex, and
  • input element type is not integer whose bitwidth is greater than 32

will convert to tosa operators as follows:

  1. if input element type is floating point, add input with constant -0.f of the broadcast shape
  2. if input element type is i1, logical-or input with constant 'false' of the broadcast shape
  3. if input element type is i32, add input with constant 0 (i32) of the broadcast shape
  4. otherwise, cast input to i32, add with constant 0 (i32) of the broadcast shape, and cast back to original element type

added tf/tfl lit tests

Change-Id: I12302adcf1c791d452a5b5a928e63e5ffcd523bc

@google-ml-butler google-ml-butler bot added the size:L CL Change Size: Large label May 24, 2023
@github-actions github-actions bot added the kokoro:force-run Tests on submitted change label May 24, 2023
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label May 24, 2023
@gbaned gbaned added this to Assigned Reviewer in PR Queue via automation May 25, 2023
@gbaned gbaned requested a review from jpienaar May 25, 2023 11:59
@google-ml-butler google-ml-butler bot added the awaiting review Pull request awaiting review label May 25, 2023
@github-actions github-actions bot added the kokoro:force-run Tests on submitted change label Jun 1, 2023
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jun 1, 2023
PR Queue automation moved this from Assigned Reviewer to Approved by Reviewer Jun 1, 2023
@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 1, 2023
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jun 1, 2023
@gbaned
Copy link
Contributor

gbaned commented Jun 12, 2023

Hi @Tai78641 Can you please take a look on below internal errors. Thank you!

TensorFlow crashed, please file a bug on https://github.com/tensorflow/tensorflow/issues with the trace below.
#0 0x0000562cb79c0f6e llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) ()
#1 0x0000562cb79bee29 llvm::sys::RunSignalHandlers()
#29 0x00007f49cbc1d69f clone (/usr/grte/v5/lib64/libc.so.6+0x13969f)
FileCheck error: '' is empty.
FileCheck command line: FileCheck /tensorflow/compiler/mlir/tosa/tests/tf-to-tosa-pipeline.mlir

@google-ml-butler google-ml-butler bot removed the ready to pull PR ready for merge process label Jun 12, 2023
@github-actions github-actions bot added the kokoro:force-run Tests on submitted change label Jun 12, 2023
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jun 12, 2023
@Tai78641
Copy link
Contributor Author

@gbaned I rebased and double checked. tf-to-tosa-pipeline.mlir passes for me.

@gbaned gbaned requested a review from jpienaar June 16, 2023 15:40
@Tai78641
Copy link
Contributor Author

@jpienaar I cannot figure out why the Py+CPP Test Suite failures.

@github-actions github-actions bot added the kokoro:force-run Tests on submitted change label Jun 30, 2023
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jun 30, 2023
@Tai78641
Copy link
Contributor Author

rebased to use dyn_cast(...) style

int32_t num_elements = 1;
SmallVector<int64_t> new_shape;
for (int i = 0; i < shape_rank; i++) {
auto shape_dim = shape_elems.getValues<IntegerAttr>()[i].getInt();
Copy link
Member

Choose a reason for hiding this comment

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

Has TOSA been moved to properties for shape?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure I understand the question, which probably means the answer is no?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think Jacques is referring to using shape_elems. Generally we try to use the getType() value as its "assumed to be true" value. Obviously this is TFLite so either the shape_elems or the getType() could be under defined. I would recommend pulling both and consolidating the correct value.

Copy link
Member

Choose a reason for hiding this comment

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

I forgot this was TFL, I was thinking about requiring IntegerAttr contruction here which could be avoided by properties.

@github-actions github-actions bot added the kokoro:force-run Tests on submitted change label Jul 10, 2023
@kokoro-team kokoro-team removed kokoro:force-run Tests on submitted change labels Jul 10, 2023

if (element_type.isa<FloatType>()) {
// F32: legalize to broadcastable Add with (-0.f)
std::vector<float> values(num_elements, -0.f);
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency you should change over to llvm::SmallVector>

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

int32_t num_elements = 1;
SmallVector<int64_t> new_shape;
for (int i = 0; i < shape_rank; i++) {
auto shape_dim = shape_elems.getValues<IntegerAttr>()[i].getInt();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think Jacques is referring to using shape_elems. Generally we try to use the getType() value as its "assumed to be true" value. Obviously this is TFLite so either the shape_elems or the getType() could be under defined. I would recommend pulling both and consolidating the correct value.

PR Queue automation moved this from Approved by Reviewer to Reviewer Requested Changes Jul 10, 2023
For tf/tfl broadcast-to operator with input and shape where:
  - shape is compile time constant, and
  - shape's rank is greater than or equal to input's rank, and
  - input element type is not complex, and
  - input element type is not integer whose bitwidth is greater than 32

will convert to tosa operators as follows:

1. if input element type is floating point, add input with
   constant -0.f of the broadcast shape
2. if input element type is i1, logical-or input with constant
   'false' of the broadcast shape
3. if input element type is i32, add input with constant 0 (i32)
   of the broadcast shape
4. otherwise, cast input to i32, add with constant 0 (i32) of the
   broadcast shape, and cast back to original element type

added tf/tfl lit tests

Signed-off-by: Tai Ly <tai.ly@arm.com>
Change-Id: I12302adcf1c791d452a5b5a928e63e5ffcd523bc
@github-actions github-actions bot added the kokoro:force-run Tests on submitted change label Jul 11, 2023
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jul 11, 2023
op,
"shape's constant value has different elements than its static "
"dimension");
return std::nullopt;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added this check that shape_elems (derived from compile time constant for input "shape") is consistent with the getType() for input "shape".

Copy link
Member

Choose a reason for hiding this comment

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

Should this be part of the op verification? (I mean I think this is TFL side, so you could add a TODO here and I could ping folks)

@Tai78641 Tai78641 requested a review from rsuderman July 11, 2023 20:53
@Tai78641
Copy link
Contributor Author

@rsuderman please have a look at this again. thanks

op,
"shape's constant value has different elements than its static "
"dimension");
return std::nullopt;
Copy link
Member

Choose a reason for hiding this comment

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

Should this be part of the op verification? (I mean I think this is TFL side, so you could add a TODO here and I could ping folks)

// reshape input to shape_rank
SmallVector<int64_t> reshaped_shape((shape_rank - input_rank), 1);
for (auto dim : input_type.getShape()) {
reshaped_shape.push_back(dim);
Copy link
Member

Choose a reason for hiding this comment

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

You could also use insert method on SmallVector. (append_range helper in LLVM could also be useful)

@google-ml-butler google-ml-butler bot added kokoro:force-run Tests on submitted change ready to pull PR ready for merge process labels Jul 18, 2023
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Jul 18, 2023
@gbaned
Copy link
Contributor

gbaned commented Jul 20, 2023

Hi @Tai78641 Can you please check @jpienaar's comments and keep us posted ? Thank you!

@copybara-service copybara-service bot merged commit aec0be7 into tensorflow:master Jul 24, 2023
10 of 14 checks passed
PR Queue automation moved this from Reviewer Requested Changes to Merged Jul 24, 2023
@rsuderman
Copy link
Contributor

I had to make some modifications to land. I believe it should still be good but you may need to validate that things are working as intended.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review Pull request awaiting review ready to pull PR ready for merge process size:L CL Change Size: Large
Projects
PR Queue
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

5 participants