[AMDGPU] Do not rewrite or approximate math functions on ROCm #19970
Conversation
bjacob
force-pushed
the
no-approx-at-all-on-rocm
branch
from
4ae6121 to
35e4441
Compare
|
If CI passes that's a good indication for e2e correctness for now I think |
|
Interesting. It has compilation failures |
| // CHECK: math.exp2 | ||
| // CHECK: math.expm1 | ||
| // CHECK: math.cbrt | ||
| // CHECK: math.erf |
There was a problem hiding this comment.
have we figured out the numerical issue with math.erf library function?
There was a problem hiding this comment.
We have (99%) figured that there was no issue with it, and the issues we ran into were caused by PolynomialApproximationPass being too coarse-grained and too convoluted, so that when we thought earlier that we were enabling/disabling math.erf approximation, we were also enabling/disabling a number of other things, unintentionally. This is what #19922 solved. Now in the present PR we are finally at a place where we have some fine-grained, well-defined levers to play with.
There was a problem hiding this comment.
The remaining issue has been diagnosed in #20074 (comment) as an issue with overly strict tests requiring agreement with the less accurate polynomial approximations in f16, as opposed to the ROCm device lib performing the approximation after upcasting to f32.
bjacob
force-pushed
the
no-approx-at-all-on-rocm
branch
from
35e4441 to
86c508b
Compare
|
For a moment I felt that I was very close to resolving this with llvm/llvm-project#128203. That PR does solve the CI issues observed here, except that I have to add all the remaining math ops to scalarization, which is not desirable. If and when we revive this, we have good reasons at this point to handle the necessary vector-flattening downstream. |
Signed-off-by: Benoit Jacob <[email protected]>
bjacob
force-pushed
the
no-approx-at-all-on-rocm
branch
from
86c508b to
2d14e5e
Compare
|
Good news, this should be unblocked by llvm/llvm-project#128915. Retrying now. |
On ROCm, we want to use the device library for all math functions.
This expands on #19969, which only concerned
math.erf.We only leave one category of rewrites enabled: the operand casts to f32. The ROCm device library internally performs the same for many math functions, but we leave that unchanged here for incrementality. We might get to that in a follow-up PR.