Skip to content

Conversation

asuhan
Copy link
Contributor

@asuhan asuhan commented Sep 11, 2020

Summary: When caller / callee pairs are inserted into the mapping, verify that
the arity of the buffer access is consistent with its declared rank.

Test Plan: CI, test_tensorexpr --gtest_filter=TensorExprTest.DetectInlineRankMismatch

@asuhan asuhan requested a review from ZolotukhinM September 11, 2020 18:54
@asuhan asuhan requested a review from apaszke as a code owner September 11, 2020 18:54
@asuhan asuhan changed the title Check for index-rank consistency FunctionInliner Check for index-rank consistency in FunctionInliner Sep 11, 2020
@ZolotukhinM ZolotukhinM requested a review from nickgg September 11, 2020 18:55
@asuhan asuhan force-pushed the asuhan/check_arity_of_inline branch 2 times, most recently from 771379b to bd3e465 Compare September 11, 2020 18:56
Copy link

@ZolotukhinM ZolotukhinM left a comment

Choose a reason for hiding this comment

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

@nickgg has been actively working on this recently, so I'd be more comfortable if he could review this

@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Sep 11, 2020
@nickgg
Copy link
Contributor

nickgg commented Sep 11, 2020

Looks fine, but this code is removed in #44231. Can you add a test to make sure that PR doesn't regress on this issue?

Summary: When caller / callee pairs are	inserted into the mapping, verify that
the arity of the buffer access is consistent with its declared rank.

Test Plan: CI, test_tensorexpr --gtest_filter=TensorExprTest.DetectInlineRankMismatch
@asuhan asuhan force-pushed the asuhan/check_arity_of_inline branch from bd3e465 to 1aba169 Compare September 11, 2020 22:04
@asuhan
Copy link
Contributor Author

asuhan commented Sep 11, 2020

@nickgg Added a test for the error path.

@codecov
Copy link

codecov bot commented Sep 12, 2020

Codecov Report

Merging #44561 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #44561   +/-   ##
=======================================
  Coverage   67.97%   67.97%           
=======================================
  Files         384      384           
  Lines       49589    49589           
=======================================
  Hits        33710    33710           
  Misses      15879    15879           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 442957d...1aba169. Read the comment docs.

Copy link
Contributor

@nickgg nickgg left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@asuhan has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@asuhan merged this pull request in a188dbd.

@asuhan asuhan deleted the asuhan/check_arity_of_inline branch September 17, 2020 23:41
xuzhao9 pushed a commit that referenced this pull request Sep 18, 2020
Summary:
When caller / callee pairs are	inserted into the mapping, verify that
the arity of the buffer access is consistent with its declared rank.

Pull Request resolved: #44561

Test Plan: CI, test_tensorexpr --gtest_filter=TensorExprTest.DetectInlineRankMismatch

Reviewed By: albanD

Differential Revision: D23684342

Pulled By: asuhan

fbshipit-source-id: dd3a0cdd4c2492853fa68381468e0ec037136cab
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Merged oncall: jit Add this issue/PR to JIT oncall triage queue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants