Skip to content

amdgcn failing to select llvm.vector.reduce.fmaximum.v2f32(<2 x float> intrinsic #67815

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

Open
nirvedhmeshram opened this issue Sep 29, 2023 · 12 comments
Labels
backend:AMDGPU bug Indicates an unexpected problem or unintended behavior

Comments

@nirvedhmeshram
Copy link
Contributor

LLVM ERROR: Cannot select: 0x1743efdb048: f32 = fmaximum # D:1 0x1743efe0e58, 0x1743efe0ec8

Detailed dump here

More context on iree-org/iree#15064

@nirvedhmeshram nirvedhmeshram added the bug Indicates an unexpected problem or unintended behavior label Sep 29, 2023
@llvmbot
Copy link
Member

llvmbot commented Sep 29, 2023

@llvm/issue-subscribers-bug

``` LLVM ERROR: Cannot select: 0x1743efdb048: f32 = fmaximum # D:1 0x1743efe0e58, 0x1743efe0ec8 ```

Detailed dump here

More context on iree-org/iree#15064

@llvmbot
Copy link
Member

llvmbot commented Sep 29, 2023

@llvm/issue-subscribers-backend-amdgpu

``` LLVM ERROR: Cannot select: 0x1743efdb048: f32 = fmaximum # D:1 0x1743efe0e58, 0x1743efe0ec8 ```

Detailed dump here

More context on iree-org/iree#15064

@nirvedhmeshram
Copy link
Contributor Author

debug dump with debug symbols here

@nirvedhmeshram
Copy link
Contributor Author

cc @krzysz00 @jayfoad

@kuhar
Copy link
Member

kuhar commented Sep 29, 2023

@jayfoad Could you take a look or help us find someone familiar with this part of the codebase?

@jayfoad
Copy link
Contributor

jayfoad commented Sep 29, 2023

I have not looked in any detail, but IR has 3 different kinds of floating point max with different rules for nans and signed zeros: fmaxnum, fmaxnum_ieee and fmaximum. The backend does not natively support fmaximum. I'm not sure whether sdag should have lowered it to something that the backend does support.

Is it possible you could switch to using a different flavor of max in the first place? Or do you really need the fmaximum semantics?

@jayfoad
Copy link
Contributor

jayfoad commented Sep 29, 2023

@arsenm is probably the most familiar with this.

@nirvedhmeshram
Copy link
Contributor Author

on the mlir side this is the change that introduced this semantics
https://reviews.llvm.org/D158618

It seems to have done after some discussions
https://discourse.llvm.org/t/rfc-fix-floating-point-max-and-min-operations-in-mlir/72671

@kuhar any thoughts how to unblock amdgpu on this? I do believe that the correct thing to do is that the backend should support thse

@kuhar
Copy link
Member

kuhar commented Sep 29, 2023

I'm not sure whether sdag should have lowered it to something that the backend does support.

@jayfoad on the spirv gl side we have to emit comparisons with nan for a similar reason. I guess the backend should be able to do something similar in sdag? #66696

@arsenm
Copy link
Contributor

arsenm commented Oct 2, 2023

Somehow nobody ever implemented legalization for maximum/minimum. There's nothing AMDGPU specific about this

@nirvedhmeshram
Copy link
Contributor Author

nirvedhmeshram commented Oct 3, 2023

I think this is just missing SD node selection on the AMDGPU side, for example the NVPTX backend selects it here

defm FMIN : F3<"min", fminnum>;
defm FMAX : F3<"max", fmaxnum>;
// Note: min.NaN.f64 and max.NaN.f64 do not actually exist.
defm FMINNAN : F3<"min.NaN", fminimum>;
defm FMAXNAN : F3<"max.NaN", fmaximum>;

@jayfoad
Copy link
Contributor

jayfoad commented Oct 10, 2023

Looks like #67301 might fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:AMDGPU bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

No branches or pull requests

6 participants