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

Check dialect changes #88

Closed
KavithaTipturMadhu opened this issue Oct 27, 2022 · 2 comments · Fixed by #145
Closed

Check dialect changes #88

KavithaTipturMadhu opened this issue Oct 27, 2022 · 2 comments · Fixed by #145
Assignees

Comments

@KavithaTipturMadhu
Copy link
Contributor

Compile the checks into calls to an assert at runtime only, and not lower the whole operator into a runtime call : This avoids weakly typing the runtime utilities in order to support different datatypes.

@KavithaTipturMadhu KavithaTipturMadhu self-assigned this Oct 27, 2022
@chelini
Copy link
Contributor

chelini commented Oct 27, 2022

Thanks, let me dump what we discussed.
check.expect_almost_eq(%arg0, %arg1, %arg3) : tensor<5xf32>, tensor<5xf32>, f32

Would be nice if we can lower this op as:

for (int i = 0; i < 5; i++) {
  %load1 = memref.load %arg0[i]
  %load2 = memref.load %arg1[i]
  if (abs(%load1 - %load2) > %th)
    @call_assert()
}

@rengolin
Copy link
Contributor

rengolin commented Nov 4, 2022

Some considerations:

  • We have expect_true, probably should also have expect_false
  • We have expect_almost_eq, should also have expect_eq (as in identical), for checking blocking and copies
  • We need to decide what to do for different precision (f32 vs bf16)
  • Do we also need expect_ne? expect_gt, expect_lt, etc?
  • Do we want to print the tensor (at least the region) when they're not eq / almost_eq?
  • What is better:
    • check.expect_almost_eq(%a, %b, %delta) or
    • %0 = check.compare_fp((%a, %b, %delta); check.expect_true(%0)? (%delta = 0 for identical)
  • If the former, then we may want to expand on the number of expects
  • If the latter, we may only have one check (expect_true, perhaps expect_false too), and use arith for everything else.
  • I still don't like the name expect_almost_eq. I think the existence of a %delta is enough to convey the meaning that it's an imprecise comparison...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants