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.

Sign up for GitHub

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

Jump to bottom

Constprop: More unary/binary ops. Also don't crash in divide by Zero. #2862

Merged
merged 11 commits into from
Aug 6, 2024
Merged

Constprop: More unary/binary ops. Also don't crash in divide by Zero. #2862

merged 11 commits into from
Aug 6, 2024

Conversation

ttjost
Copy link
Contributor

@ttjost ttjost commented Jul 3, 2024

No description provided.

@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

@ttjost ttjost force-pushed the tiagot.constprop_for_many_unary_binary_ops_upstream branch from 9816bdd to 201dbf4 Compare July 3, 2024 11:35
@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

@ttjost ttjost force-pushed the tiagot.constprop_for_many_unary_binary_ops_upstream branch from 201dbf4 to 2cec11f Compare July 3, 2024 11:44
@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

@ttjost ttjost force-pushed the tiagot.constprop_for_many_unary_binary_ops_upstream branch from 2cec11f to 0d39a93 Compare July 3, 2024 11:45
@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

@ttjost ttjost force-pushed the tiagot.constprop_for_many_unary_binary_ops_upstream branch from 0d39a93 to a971fda Compare July 3, 2024 11:48
@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

@ttjost ttjost force-pushed the tiagot.constprop_for_many_unary_binary_ops_upstream branch from a971fda to a40137e Compare July 3, 2024 11:52
@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

ttjost and others added 2 commits July 3, 2024 13:55
@ttjost
feat: add constant propagation for different unary and binary ops.
eab175a 
Signed-off-by: Tiago Trevisan Jost <tiago.trevisanjost@amd.com>
@mgehre-amd @ttjost
Don't crash when constant folding an integer division by zero.
670636f 
Signed-off-by: Tiago Trevisan Jost <tiago.trevisanjost@amd.com>
Co-authored-by: Matthias Gehre <matthias.gehre@amd.com>
Signed-off-by: Tiago Trevisan Jost <tiago.trevisanjost@amd.com>
@ttjost ttjost force-pushed the tiagot.constprop_for_many_unary_binary_ops_upstream branch from a40137e to 670636f Compare July 3, 2024 11:55
@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

AlexandreEichenberger
AlexandreEichenberger approved these changes Jul 3, 2024
View reviewed changes
Copy link
Collaborator

@AlexandreEichenberger AlexandreEichenberger left a comment

Choose a reason for hiding this comment

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

Thanks for adding all the constant propagation rules for more operations, very useful.
LGTM

@AlexandreEichenberger
Copy link
Collaborator

@tungld @flemairen6 If we add new operators to generate constants (like in this PR), do we also need to add new operators to the"lazy creation" of our constant values, to ensure we don't blow up our memory requirement for large benchmarks?

It would be nice to have some clarity on this.

@AlexandreEichenberger
Copy link
Collaborator

@jenkins-droid please test this

@AlexandreEichenberger
Copy link
Collaborator

@flemairen6 you have rights to start the Jenkins, correct?

@flemairen6
Copy link
Collaborator

flemairen6 commented Jul 3, 2024 •
edited
Loading

@AlexandreEichenberger I don't think I have

@AlexandreEichenberger
Copy link
Collaborator

Looks line an error in this lit test

Exit Code: 1

Command Output (stderr):
--
RUN: at line 1: /workdir/onnx-mlir/build/Release/bin/onnx-mlir-opt --shape-inference --constprop-onnx /workdir/onnx-mlir/test/mlir/onnx/onnx_constprop.mlir -split-input-file | /workdir/llvm-project/build/bin/FileCheck /workdir/onnx-mlir/test/mlir/onnx/onnx_constprop.mlir
+ /workdir/onnx-mlir/build/Release/bin/onnx-mlir-opt --shape-inference --constprop-onnx /workdir/onnx-mlir/test/mlir/onnx/onnx_constprop.mlir -split-input-file
+ /workdir/llvm-project/build/bin/FileCheck /workdir/onnx-mlir/test/mlir/onnx/onnx_constprop.mlir
/workdir/onnx-mlir/test/mlir/onnx/onnx_constprop.mlir:390:12: error: CHECK: expected string not found in input
 // CHECK: onnx.Constant dense<{{.}}[-2.765630e+00, 0xFFC0], [0xFF80, 0x7FC0], [0x7F80, 0xFFC0]]>
           ^
<stdin>:244:43: note: scanning from here
 func.func @test_log() -> tensor<3x2xbf16> {                                     ^

If you don't see the error in your normal build environment, try building the RELEASE mode, as it sometimes expose errors.

@AlexandreEichenberger
Copy link
Collaborator

@AlexandreEichenberger I don't think I have

will ask for you to be added.

tungld
tungld reviewed Jul 4, 2024
View reviewed changes
Copy link
Collaborator

@tungld tungld left a comment

Choose a reason for hiding this comment

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

Thank you for adding support for many ops!

@tungld
Copy link
Collaborator

tungld commented Jul 4, 2024

do we also need to add new operators to the"lazy creation" of our constant values, to ensure we don't blow up our memory requirement for large benchmarks?

Existing templates in ConstProp can handle constant values in a lazy way. If we do manually, just to make sure to use the OnnxElementsAttrBuilder class and ElementsAttrHelper functions to construct and access ElementsAttr. More info can be found here: https://github.com/onnx/onnx-mlir/blob/main/docs/ConstPropagationPass.md

@AlexandreEichenberger
Copy link
Collaborator

Existing templates in ConstProp can handle constant values in a lazy way. If we do manually, just to make sure to use the OnnxElementsAttrBuilder class and ElementsAttrHelper functions to construct and access ElementsAttr. More info can be found here: https://github.com/onnx/onnx-mlir/blob/main/docs/ConstPropagationPass.md

@tungld thanks for the link. The doc seems to be out of date, as a grep of combinerOfElementwiseBinaryOp (which is described as the method to develop a combiner, which would be presumably need to be added for the new ops) yielded only hits in the doc, and not the code.

@AlexandreEichenberger
Copy link
Collaborator

@ttjost the PR is ready to be merged but there are some conflicts that needs to be manually merged. Do you have the time to do that last step?

ttjost added 2 commits July 25, 2024 19:58
@ttjost
Merge branch 'main' of https://github.com/onnx/onnx-mlir into tiagot.…
d7698fa 
…constprop_for_many_unary_binary_ops_upstream
@ttjost
chore: use numpy's definition for when min > max in clip during const…
b61d6e2 
…ant propagation.

Signed-off-by: Tiago Trevisan Jost <tiago.trevisanjost@amd.com>
@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

@ttjost
Copy link
Contributor Author

ttjost commented Jul 25, 2024 •
edited
Loading

@ttjost the PR is ready to be merged but there are some conflicts that needs to be manually merged. Do you have the time to do that last step?

Apologies for the delay. I made some changes as suggested, and now we use the same approach numpy uses for clip (When a_min is greater than a_max, returns an array in which all values are equal to a_max, as shown in the second example. (https://numpy.org/doc/stable/reference/generated/numpy.clip.html)

@ttjost
Copy link
Contributor Author

ttjost commented Jul 26, 2024

@AlexandreEichenberger, could you please start Jenkins?

@AlexandreEichenberger
Copy link
Collaborator

@jenkins-droid test this please

ttjost added 2 commits August 2, 2024 07:08
@ttjost
chore: fix LIT test issue with log.
ce58f6e 
Signed-off-by: Tiago Trevisan Jost <tiago.trevisanjost@amd.com>
@ttjost
Merge branch 'tiagot.constprop_for_many_unary_binary_ops_upstream' of h…
df5a726 
…ttps://github.com/Xilinx/onnx-mlir into tiagot.constprop_for_many_unary_binary_ops_upstream
@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

@tungld
Copy link
Collaborator

tungld commented Aug 2, 2024

@jenkins-droid test this please

@ttjost
chore: fix LIT test issues with sin/cos.
eddc5f9 
Signed-off-by: Tiago Trevisan Jost <tiago.trevisanjost@amd.com>
@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

@tungld
Copy link
Collaborator

tungld commented Aug 2, 2024

@jenkins-droid test this please

@ttjost
Copy link
Contributor Author

ttjost commented Aug 2, 2024

Thanks @tungld . I can't reproduce locally the issues I am seeing in the CI, but it relates to different implementations of some math functions when returning NaN values. Locally and in a local CI I get negative NaN, and in Onnx-MLIR CI it returns me a positive NaN. Hopefully, this now fixes the issues.

@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

@ttjost
Copy link
Contributor Author

ttjost commented Aug 2, 2024

FYI, needed to update the branch but all tests passed previously.

@flemairen6
Copy link
Collaborator

@jenkins-droid test this please

@ttjost
Copy link
Contributor Author

ttjost commented Aug 2, 2024

PR is ready to merge, but I don't have write access.
Could someone handle it, please? Thanks!

@AlexandreEichenberger
Copy link
Collaborator

@ttjost I would, for some reason it shows at "out of date" and the usual button to bring it up to date is not present. Can you merge the latest?

@jenkins-droid
Copy link
Collaborator

Can one of the admins verify this patch?

@tungld
Copy link
Collaborator

tungld commented Aug 5, 2024

@jenkins-droid test this please

@ttjost
Copy link
Contributor Author

ttjost commented Aug 6, 2024 •
edited
Loading

could someone please merge this PR? Thanks!

@tungld tungld merged commit f11a21c into onnx:main Aug 6, 2024
7 checks passed
@tungld
Copy link
Collaborator

tungld commented Aug 6, 2024

could someone please merge this PR? Thanks!

Merged. Thank you very much for the PR!

@jenkins-droid
Copy link
Collaborator

Jenkins Linux s390x Build #15278 [push] Constprop: More unary/bi... started at 03:41

@jenkins-droid
Copy link
Collaborator

Jenkins Linux ppc64le Build #14303 [push] Constprop: More unary/bi... started at 03:53

@jenkins-droid
Copy link
Collaborator

Jenkins Linux amd64 Build #15273 [push] Constprop: More unary/bi... started at 02:41

@jenkins-droid
Copy link
Collaborator

Jenkins Linux amd64 Build #15273 [push] Constprop: More unary/bi... passed after 1 hr 12 min

@jenkins-droid
Copy link
Collaborator

Jenkins Linux s390x Build #15278 [push] Constprop: More unary/bi... passed after 1 hr 42 min

@jenkins-droid
Copy link
Collaborator

Jenkins Linux ppc64le Build #14303 [push] Constprop: More unary/bi... passed after 2 hr 8 min

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tungld tungld tungld left review comments

@AlexandreEichenberger AlexandreEichenberger AlexandreEichenberger approved these changes

@mgehre-amd mgehre-amd mgehre-amd left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ttjost @jenkins-droid @AlexandreEichenberger @flemairen6 @tungld @mgehre-amd