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

Fix missing round(), floor(), ceil() for target C lowering #7382

Merged
merged 1 commit into from Feb 2, 2021
Merged

Fix missing round(), floor(), ceil() for target C lowering #7382

merged 1 commit into from Feb 2, 2021

Conversation

cbalint13
Copy link
Contributor

  • Trying to lower quantized C target some simple nets reveal missing round, floor, ceil translation.
   with relay.build_config(opt_level=3):
      graph, c_mod, c_params = relay.build(mod, target='c', params=params)
  • Network looks like the one below, notice for example the round operations:
#[version = "0.0.5"]
def @main(%input_3: Tensor[(1, 1, 129, 124), float32]) -> Tensor[(?, 8), float32] {
  %0 = dyn.image.resize(%input_3, meta[relay.Constant][0] /* ty=Tensor[(2), int64] */, size=[]) /* ty=Tensor[(1, 1, ?, ?), float32] */;
  %1 = multiply(%0, 16f /* ty=float32 */) /* ty=Tensor[(1, 1, ?, ?), float32] */;
  %2 = round(%1) /* ty=Tensor[(1, 1, ?, ?), float32] */;
  .....
  .....
  • The errors encountered:
TVMError: Unresolved call Op(tir.round)
TVMError: Unresolved call Op(tir.floor)
TVMError: Unresolved call Op(tir.ceil)

  • This simple patch address the issue.

@jroesch @masashi @mdw-octoml Please help me review.

Thank you !

@mdw-octoml
Copy link
Contributor

LGTM, thanks! @areusch

Copy link
Contributor

@manupak manupak 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 the PR! a much needed one :).

one suggestion and I think it would better if we can have some test cases in tests/python/unittest/test_target_codegen_c_host.py to cover these.

src/target/source/codegen_c.cc Outdated Show resolved Hide resolved
@areusch
Copy link
Contributor

areusch commented Feb 1, 2021

thanks @cbalint13! this looks good but would be great if you could write a test case to prevent regressions in the future. I think the tests/python/unittest/test_target_codegen_c_host.py is a good place for that.

src/target/source/codegen_c.cc Outdated Show resolved Hide resolved
@tqchen tqchen added the status: need update need update based on feedbacks label Feb 1, 2021
@cbalint13 cbalint13 changed the title Add missing round(), floor(), ceil() to codegen_c.cc backend Fix missing round(), floor(), ceil() for target C lowering Feb 2, 2021
@cbalint13
Copy link
Contributor Author

@mdw-octoml , @areusch , @manupa-arm

  • Added missing to target/intrin_rule.cc (as @tqchen suggested).
  • Added test cases to check C lowering.
  • Prepended test_ local funcs of testunit avoiding name conflict with <math.h>
  • C generated code is properly lowered to C function calls:
((float*)B)[(i0)] = ceilf(((float*)A)[(i0)]);
((float*)B)[(i0)] = floorf(((float*)A)[(i0)]);
((float*)B)[(i0)] = roundf(((float*)A)[(i0)]);

@cbalint13 cbalint13 requested a review from tqchen February 2, 2021 16:17
Copy link
Contributor

@manupak manupak left a comment

Choose a reason for hiding this comment

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

LGTM

@tqchen tqchen merged commit de0ab4c into apache:main Feb 2, 2021
@tqchen
Copy link
Member

tqchen commented Feb 2, 2021

@tqchen tqchen added status: accepted and removed status: need update need update based on feedbacks labels Feb 2, 2021
electriclilies pushed a commit to electriclilies/tvm that referenced this pull request Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants