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

[mlir] Lit tests fail after BF16 ABI change #57042

Closed
phoebewang opened this issue Aug 10, 2022 · 7 comments
Closed

[mlir] Lit tests fail after BF16 ABI change #57042

phoebewang opened this issue Aug 10, 2022 · 7 comments
Assignees
Labels
mlir:sparse Sparse compiler in MLIR

Comments

@phoebewang
Copy link
Contributor

I had a patch c7ec6e1 to make the bfloat type to follow X86 psABI. It seems the change makes MLIR tests failed again:

Failed Tests (2):
  MLIR :: Integration/Dialect/SparseTensor/CPU/dense_output_bf16.mlir
  MLIR :: Integration/Dialect/SparseTensor/CPU/sparse_sum_bf16.mlir

I suppose it is the same reason as #55992. cc @joker-eph @aartbik do I need to disable them temporarily, or you have a quick fix? Please notice, I'm planning to backport the change to LLVM 15.0 too.

@llvmbot
Copy link
Collaborator

llvmbot commented Aug 10, 2022

@llvm/issue-subscribers-mlir

@aartbik
Copy link
Contributor

aartbik commented Aug 10, 2022

The ABI issue was supposed to be fixed by https://reviews.llvm.org/D128018 which passes the value through memory (by reference rather than by value). If it breaks again I really would like to know why and not disable the tests anymore.

@aartbik aartbik self-assigned this Aug 10, 2022
@aartbik
Copy link
Contributor

aartbik commented Aug 10, 2022

Discussed this with @joker-eph. For the sake of progress, I am okay disabling the tests and investigating what happened "offline".

aartbik added a commit that referenced this issue Aug 10, 2022
Supposedly our ABI issues were fixed, per issue:
#55992

However, with a recent changes to bf16, these tests
fail again; not sure why yet:
https://reviews.llvm.org/D130832
https://lab.llvm.org/buildbot/#/builders/61/builds/30600

So we disable the tests for now. Issue is tracked in:
#57042

Differential Revision: https://reviews.llvm.org/D131621
@phoebewang
Copy link
Contributor Author

Thanks @joker-eph and @aartbik . I think the problem is in the runtimes that still expect BF16 type passed as uint16_t. I think changing it to __bf16 or even float for X86 targets should solve the problem.
https://github.com/llvm/llvm-project/blob/main/mlir/lib/ExecutionEngine/Float16bits.cpp#L149-L175

@phoebewang
Copy link
Contributor Author

FYI. This is what I changed in compiler-rt https://reviews.llvm.org/D131147

@d0k d0k closed this as completed in f695554 Aug 11, 2022
llvmbot pushed a commit to llvm/llvm-project-release-prs that referenced this issue Aug 11, 2022
Supposedly our ABI issues were fixed, per issue:
llvm/llvm-project#55992

However, with a recent changes to bf16, these tests
fail again; not sure why yet:
https://reviews.llvm.org/D130832
https://lab.llvm.org/buildbot/#/builders/61/builds/30600

So we disable the tests for now. Issue is tracked in:
llvm/llvm-project#57042

Differential Revision: https://reviews.llvm.org/D131621

(cherry picked from commit 6b74591)
@EugeneZelenko EugeneZelenko added mlir:sparse Sparse compiler in MLIR and removed mlir labels Aug 11, 2022
@llvmbot
Copy link
Collaborator

llvmbot commented Aug 11, 2022

@llvm/issue-subscribers-mlir-sparse

llvmbot pushed a commit to llvm/llvm-project-release-prs that referenced this issue Aug 11, 2022
Supposedly our ABI issues were fixed, per issue:
llvm/llvm-project#55992

However, with a recent changes to bf16, these tests
fail again; not sure why yet:
https://reviews.llvm.org/D130832
https://lab.llvm.org/buildbot/#/builders/61/builds/30600

So we disable the tests for now. Issue is tracked in:
llvm/llvm-project#57042

Differential Revision: https://reviews.llvm.org/D131621

(cherry picked from commit 6b74591)
@aartbik
Copy link
Contributor

aartbik commented Aug 11, 2022

Thanks @d0k for the fix in f695554

tru pushed a commit to llvm/llvm-project-release-prs that referenced this issue Aug 12, 2022
Supposedly our ABI issues were fixed, per issue:
llvm/llvm-project#55992

However, with a recent changes to bf16, these tests
fail again; not sure why yet:
https://reviews.llvm.org/D130832
https://lab.llvm.org/buildbot/#/builders/61/builds/30600

So we disable the tests for now. Issue is tracked in:
llvm/llvm-project#57042

Differential Revision: https://reviews.llvm.org/D131621

(cherry picked from commit 6b74591)
tru pushed a commit that referenced this issue Aug 12, 2022
c7ec6e1 made LLVM adhere to the x86
psABI and pass bf16 in SSE registers instead of GPRs. This breaks the
custom versions of runtime functions we have for bf16 conversion. A
great fix for this would be to use __bf16 types instead which carry the
right ABI, but that type isn't widely available.

Instead just pretend it's a 32 bit float on the ABI boundary and
carefully cast it to the right type.

Fixes #57042

(cherry picked from commit f695554)
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this issue Oct 7, 2022
Supposedly our ABI issues were fixed, per issue:
llvm/llvm-project#55992

However, with a recent changes to bf16, these tests
fail again; not sure why yet:
https://reviews.llvm.org/D130832
https://lab.llvm.org/buildbot/#/builders/61/builds/30600

So we disable the tests for now. Issue is tracked in:
llvm/llvm-project#57042

Differential Revision: https://reviews.llvm.org/D131621

(cherry picked from commit 6b7459115f7b1c43f81ad8dfb5da9d206d3b1e06)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
mlir:sparse Sparse compiler in MLIR
Projects
None yet
Development

No branches or pull requests

4 participants