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.
Paths
| Differential D101727
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 TimelineComment Actions The current code was added with rG8b6d9f6 and a reaction/partial fix for: 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.
Comment Actions FYI just added support for select w/ fast-math to Alive2 so we can check these tests. Comment Actions
Great! Is it available on the online instance yet? 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?
Comment Actions
Your example is correct, yes! Thanks for checking.
Krishnakariya marked 2 inline comments as done. Comment ActionsI have made changes in FMF propagation logic. 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) Comment Actions This patch is complete and is ready for review.
This example is a new bug detected by Alive 2 after introducing and testing this patch. Comment Actions 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? Comment Actions
I am sorry for the confusion. This version has few issues which were detected by Alive2. So, I have made new changes as follows:
Comment Actions
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface
Krishnakariya marked an inline comment as done. Comment ActionsUpdates:
Krishnakariya marked an inline comment as done. Comment Actions Ping @spatel, a gentle reminder. Could you please review the updates on this patch?
Comment Actions
I think you missed my earlier comment: 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? Comment Actions Hi @spatel, Comment Actions
Yes, that looks like better coverage. We'll need to adjust some test comments too. Comment Actions 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.
This revision is now accepted and ready to land.Jul 23 2021, 8:36 AM Comment Actions Yes, we may remove FMF checks as below. I will make a new patch for this. 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 Closed by commit rG7bd361200a7b: [InstCombine] Fix PR47960 - Incorrect transformation of fabs with nnan flag (authored by Krishnakariya, committed by spatel). · Explain Why This revision was automatically updated to reflect the committed changes.
Revision Contents
Diff 361508 llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp
llvm/test/Transforms/InstCombine/fabs.ll
|
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.