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

[Attributor] Incorrect optimization of math function when attributor module pass is run #78507

Closed
jhuber6 opened this issue Jan 17, 2024 · 2 comments · Fixed by #91030
Closed
Assignees
Labels

Comments

@jhuber6
Copy link
Contributor

jhuber6 commented Jan 17, 2024

The Attributor module pass currently optimized out this math function incorrectly to always return NaN. The reproducer is at https://godbolt.org/z/1P9xTjoab. When the attributor is not run, the function remains correct, when it's run it gets incorrectly removed.

@jdoerfert
Copy link
Member

The way I see it, we currently follow all uses through intrinsics and use the fpclass derived for the result also for the argument. That is wrong, e.g. the fabs result is more constrained then the argument is, I think.

I have a fix here: https://github.com/jdoerfert/llvm-project/tree/fpclass_fixes
But it needs to be refined (potentially after landing) to allow specific deductions.
I did not update the tests, even though they are impacted, because I wanted to first update them w/o the change and avoid churn. That resulted in me finding #78517.
Once @arsenm comments, we can move ahead with the fixes.

@arsenm
Copy link
Contributor

arsenm commented Jan 26, 2024

I think I just ran into another manifestation of this issue where this is incorrectly adding nofpclass to an incoming function argument based on an unread use context

@arsenm arsenm added the floating-point Floating-point math label Feb 6, 2024
jdoerfert added a commit to jdoerfert/llvm-project that referenced this issue May 3, 2024
The old must-be-executed-context (MBEC) propagation did propagate
through calls even if that was not allowed. We now only propagate from
call site arguments. If there are calls/intrinsics that allows
propagation, we need to add them explicitly.

Fixes: llvm#78507
jdoerfert added a commit to jdoerfert/llvm-project that referenced this issue May 3, 2024
The old must-be-executed-context (MBEC) propagation did propagate
through calls even if that was not allowed. We now only propagate from
call site arguments. If there are calls/intrinsics that allows
propagation, we need to add them explicitly.

Fixes: llvm#78507
jdoerfert added a commit to jdoerfert/llvm-project that referenced this issue May 22, 2024
The old must-be-executed-context (MBEC) propagation did propagate
through calls even if that was not allowed. We now only propagate from
call site arguments. If there are calls/intrinsics that allows
propagation, we need to add them explicitly.

Fixes: llvm#78507
jdoerfert added a commit to jdoerfert/llvm-project that referenced this issue Jun 3, 2024
The old must-be-executed-context (MBEC) propagation did propagate
through calls even if that was not allowed. We now only propagate from
call site arguments. If there are calls/intrinsics that allows
propagation, we need to add them explicitly.

Fixes: llvm#78507
jdoerfert added a commit that referenced this issue Jun 4, 2024
The old use of must-be-executed-context (MBEC) did propagate
through calls even if that was not allowed. We now only propagate from
call site arguments. If there are calls/intrinsics that allows
propagation, we need to add them explicitly.

Fixes: #78507

---------

Co-authored-by: Matt Arsenault <arsenm2@gmail.com>
vedantparanjape-amd pushed a commit to vedantparanjape-amd/llvm-project that referenced this issue Jun 7, 2024
The old use of must-be-executed-context (MBEC) did propagate
through calls even if that was not allowed. We now only propagate from
call site arguments. If there are calls/intrinsics that allows
propagation, we need to add them explicitly.

Fixes: llvm#78507

---------

Co-authored-by: Matt Arsenault <arsenm2@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants