This is an archive of the discontinued LLVM Phabricator instance.

Fix PR47960 - Incorrect transformation of fabs with nnan flag
ClosedPublic

Authored by Krishnakariya on May 2 2021, 1:25 PM.

Details

Summary

Bug Fix for PR: https://bugs.llvm.org/show_bug.cgi?id=47960

This patch makes sure that the fast math flag used in the 'select' instruction is the same as the 'fabs' instruction after the transformation.

Diff Detail

Event Timeline

Krishnakariya created this revision.May 2 2021, 1:25 PM
Krishnakariya requested review of this revision.May 2 2021, 1:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 2 2021, 1:25 PM
spatel added a comment.May 3 2021, 5:54 AM

The current code was added with rG8b6d9f6 and a reaction/partial fix for:
https://llvm.org/PR38086

I was afraid that relying on the select for FMF could hinder further optimizations because propagation of FMF to select was (and still is) incomplete.
But I don't have any clear evidence to show the effect either way, so I think it's fine to give this a try. Just be on the lookout for regressions.

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
2868–2869

Remove the FIXME code comment.

llvm/test/Transforms/InstCombine/fabs.ll
268

Add a test comment here to make it explicit that nnan knowledge is lost with the transform.

nlopes added a comment.May 8 2021, 7:12 AM

FYI just added support for select w/ fast-math to Alive2 so we can check these tests.

Added description for the test case.

FYI just added support for select w/ fast-math to Alive2 so we can check these tests.

Great! Is it available on the online instance yet?
Checking my understanding on this example:
https://alive2.llvm.org/ce/z/M2Fe49

define double @src(double %x) {
%0:
  %lezero = fcmp ole double %x, 0.000000
  %negx = fsub nnan double 0.000000, %x
  %fabs = select i1 %lezero, double %negx, double %x
  ret double %fabs
}
=>
define double @tgt(double %x) {
%0:
  %fabs = fabs nnan double %x
  ret double %fabs
}
Transformation doesn't verify!

ERROR: Target is more poisonous than source

Example:
double %x = NaN

Source:
i1 %lezero = #x0 (0)
double %negx = poison
double %fabs = NaN

Target:
double %fabs = poison
Source value: NaN
Target value: poison

In the source function, if we have "nnan" on the select, and we choose %x (NaN), then the return value must be poison? The "nnan" on the select was dropped in the output pane though?

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
2883

What about this one and the fneg transforms below?
If the FMF propagation logic is identical, we should change all of these simultaneously to keep things consistent.
We already have regression tests for all 4 variants (but you might want to add extra tests to make sure we're not losing coverage).

nlopes added a subscriber: regehr.May 14 2021, 8:41 AM

FYI just added support for select w/ fast-math to Alive2 so we can check these tests.

Great! Is it available on the online instance yet?
Checking my understanding on this example:
https://alive2.llvm.org/ce/z/M2Fe49

define double @src(double %x) {
%0:
  %lezero = fcmp ole double %x, 0.000000
  %negx = fsub nnan double 0.000000, %x
  %fabs = select i1 %lezero, double %negx, double %x
  ret double %fabs
}
=>
define double @tgt(double %x) {
%0:
  %fabs = fabs nnan double %x
  ret double %fabs
}
Transformation doesn't verify!

ERROR: Target is more poisonous than source

Example:
double %x = NaN

Source:
i1 %lezero = #x0 (0)
double %negx = poison
double %fabs = NaN

Target:
double %fabs = poison
Source value: NaN
Target value: poison

In the source function, if we have "nnan" on the select, and we choose %x (NaN), then the return value must be poison? The "nnan" on the select was dropped in the output pane though?

Your example is correct, yes! Thanks for checking.
There are actually two issues:

  • There was a typo in the select fast-math implementation, which I've just fixed.
  • The online version hasn't been updated in a while. 'nnan' should appear in the output, so the online version is before the fast-math changes. Pinging @regehr so he updates the online Alive2 when he has time.
Krishnakariya marked 2 inline comments as done.

I have made changes in FMF propagation logic.
FMF like nnan and ninf should be propagated only if present in select instruction, while for other flags like arcp, reassoc, it doesn't really depend on select instruction.

Another thing that I want to point out is that after dropping nnan flag, the below test encounters an error.

Example link: https://alive2.llvm.org/ce/z/YEcHxG

define double @select_fcmp_nnan_nsz_olt_zero(double %x) {
%0:
  %ltzero = fcmp olt double %x, 0.000000
  %negx = fsub nnan nsz double -0.000000, %x
  %fabs = select i1 %ltzero, double %negx, double %x
  ret double %fabs
}
=>
define double @select_fcmp_nnan_nsz_olt_zero(double %x) {
%0:
  %1 = fabs nsz double %x
  ret double %1
}
Transformation doesn't verify!
ERROR: Value mismatch

Example:
double %x = #x8000000000000000 (-0.0)

Source:
i1 %ltzero = #x0 (0)
double %negx = #x0000000000000000 (+0.0)
double %fabs = #x8000000000000000 (-0.0)

Target:
double %1 = #x0000000000000000 (+0.0)
Source value: #x8000000000000000 (-0.0)
Target value: #x0000000000000000 (+0.0)

This patch is complete and is ready for review.

Another thing that I want to point out is that after dropping nnan flag, the below test encounters an error.

Example link: https://alive2.llvm.org/ce/z/YEcHxG

This example is a new bug detected by Alive 2 after introducing and testing this patch.

I'm confused. This patch started by just switching the FMF propagation source from the fneg/fsub to the select. Now, we are back to taking flags from fneg and then intersecting some flags from the select.

But this approach has known bugs, as you have shown in the new Alive2 example. Why is this version of the patch better than just propagating all flags from the select?

I'm confused. This patch started by just switching the FMF propagation source from the fneg/fsub to the select. Now, we are back to taking flags from fneg and then intersecting some flags from the select.

But this approach has known bugs, as you have shown in the new Alive2 example. Why is this version of the patch better than just propagating all flags from the select?

I am sorry for the confusion. This version has few issues which were detected by Alive2. So, I have made new changes as follows:

  1. flags are propagated if present in select instruction.
  2. FSub logic: Removed the check for nnan's flag presence in fsub instruction for doing the optimization. Example: https://alive2.llvm.org/ce/z/tYeZub (fsub).
  3. FNeg logic: Instead of checking the presence of nnan and nsz flag in fneg instruction, added a check for nsz instruction in select instruction. Example: https://alive2.llvm.org/ce/z/TbuLnk (fneg), https://alive2.llvm.org/ce/z/yBDCZ_ (fsub).
spatel added a comment.Jul 5 2021, 9:28 AM
  1. Please upload patch with full context:

https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

  1. I prefer that we add more tests rather than alter the FMF on the existing tests. We want to be able to verify that all of these flag permutations are behaving as expected. We can commit the baseline tests as a preliminary step, so we just see the diffs in this review. If you do not have commit access, let me know so I can commit for you.
llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
2867–2868

If we are dropping the nnan requirement, this comment should be removed. Can we make that change independently of the change that propagates flags from the select without breaking anything? If so, let's do that first as its own patch before this one.

Krishnakariya marked an inline comment as done.

Updates:

  1. Added check for nnan (earlier removed) and added new tests.
  2. Check for nsz in select for fneg logic is required. So, I didn't remove this check. Please see this example: https://alive2.llvm.org/ce/z/sEfTYx.
Krishnakariya marked an inline comment as done.
Krishnakariya added a comment.EditedJul 15 2021, 2:58 AM

Ping @spatel, a gentle reminder. Could you please review the updates on this patch?

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
2867–2868

If we are dropping the nnan requirement, this comment should be removed. Can we make that change independently of the change that propagates flags from the select without breaking anything? If so, let's do that first as its own patch before this one.

Ping @spatel, a gentle reminder. Could you please review the updates on this patch?

I think you missed my earlier comment:
I prefer that we add more tests rather than alter the FMF on the existing tests. We want to be able to verify that all of these flag permutations are behaving as expected. We can commit the baseline tests as a preliminary step, so we just see the diffs in this review. If you do not have commit access, let me know so I can commit for you.

I'd like to have a final patch with no changes to the IR in the original tests; that is, we should still have test coverage for the existing patterns before this patch (and they will show that the transform does not fire any more in several cases). Does that make sense?

Hi @spatel,
I am sorry for my delayed responses to this patch. I wanted to ask for some clarification on your previous comments.
I have created few new tests similar to the original ones, with the only change that the FMF flag present in the fsub/fneg is now present on the select instruction. Original tests are left untouched.
Is this what you were expecting in your last comment?

Hi @spatel,
I am sorry for my delayed responses to this patch. I wanted to ask for some clarification on your previous comments.
I have created few new tests similar to the original ones, with the only change that the FMF flag present in the fsub/fneg is now present on the select instruction. Original tests are left untouched.
Is this what you were expecting in your last comment?

Yes, that looks like better coverage. We'll need to adjust some test comments too.
Please commit the extra tests with current CHECK line output (no code changes) to main. Then update this patch on top of that, so we will see the diffs.

spatel accepted this revision.Jul 23 2021, 8:36 AM

LGTM - I think this is safe now and seems to fix the bugs noted by Alive2 , but we really should follow-up by cleaning this up: we don't need FMF at all in some cases, and we don't need to worry about preserving sign of NaN in the default FP environment.

llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
2873

We could ease this condition, right?
If either of the fsub or the select has nnan, then the result can have nnan. It seems we don't need any FMF at all on some of these patterns:
https://alive2.llvm.org/ce/z/3bfTSb

Let's not complicate this patch any more though. If that's correct, then let's make changes as a follow-up patch (with more tests to confirm).

This revision is now accepted and ready to land.Jul 23 2021, 8:36 AM
Krishnakariya marked an inline comment as done.Jul 24 2021, 12:26 PM

Yes, we may remove FMF checks as below. I will make a new patch for this.
FSub logic: Remove check for nnan flag presence in fsub instruction. Example: https://alive2.llvm.org/ce/z/tYeZub (fsub).
FNeg logic: Remove check for the presence of nnan and nsz flag in fneg instruction. Example: https://alive2.llvm.org/ce/z/TbuLnk (fneg), https://alive2.llvm.org/ce/z/yBDCZ_ (fsub).

Could you please commit this patch on my behalf? I do not have commit access yet.

This revision was landed with ongoing or failed builds.Jul 25 2021, 7:43 AM
This revision was automatically updated to reflect the committed changes.