[PATCH] Flag to enable IEEE-754 friendly FP optimizations
Sergey Dmitrouk
sdmitrouk at accesssoftek.com
Tue Feb 10 10:12:13 PST 2015
Attached is alternative fix for InstCombine hanging, it disables only a
couple of conversions of "fadd" into "fsub" to break the loop.
Modifying dyn_castFNegVal() directly causes some unit tests to fail, so
absence of checks seems to be intensional to enable more transformations.
As there exist "fsub" to "fadd" transformations and vice versa, making
some forms as canonical would disable at least one of them. So the
patch disables replacements of "fadd" with "fsub" for "fadd"
instructions marked with floating-point environment access flags.
Regards,
Sergey
On Thu, Feb 05, 2015 at 07:56:56PM +0200, Sergey Dmitrouk wrote:
> Hello,
>
> Thanks for reviewing this.
>
> On Wed, Feb 04, 2015 at 10:42:35AM -0800, Mehdi Amini wrote:
> > Patch 1:
> >
> > The comment for:
> > + unsigned AllowFPExceptAccess : 1;
> > mentions rounding and the comment for:
> > + unsigned AllowFPRoundAccess : 1;
> > mentions exceptions, they are reversed.
>
> Updated.
>
> > Patch 2:
> >
> > Is kexc or kround flag allowed to be present at the same time as fast?
>
> Yes, currently they are. There is no particular reason for this except
> for me not being able to decide whether "fast" should reset new flags or
> not. Left it unrelated to each other as they are unlikely to be used at
> the same time, ready to change it if you have any suggestions.
>
> > Patch 3:
> >
> > isSafeToOptimizeFPOp() is not clear to me, especially how TLI->hasFloatingPointExceptions() disable any check, including rounding, while the description of this flag is "Whether the target supports or cares about preserving floating point exception behavior”, which seems orthogonal to getTarget().Options.AllowFPRoundAccess?
> > I assume it is done this way to keep existing behavior? Is it transitional?
>
> That's a mistake, it was fine until recent change for rounding when I
> reordered if statements incorrectly. Thanks.
>
> > Patch 4:
> >
> > isSafeToSpeculativelyExecute(), comment might deserve to be updated , I don’t think it is true anymore with respect to rounding:
> > "Return true if the instruction does not have any effects besides calculating the result and does not have undefined behavior”.
>
> Updated.
>
> > canTrap(), again preserving the dynamic “rounding” is included in “trap”? Maybe the comment could be updated because it does not seem obvious to me.
>
> Actually, rounding flag wasn't even used and exceptions flag was
> unnecessary, so I just reverted that part. It was required with flags
> being detached from instructions, but now that they are part of
> fast-math flags checking constant expressions should be no-operation
> (operations that go against flags must not be converted to constant
> expressions).
>
> > Patch 10: if you really want to test it, maybe the C++ unittests can do the job?
>
> Thanks, added a unit-test and a correction for "frem" instruction.
>
> > Patch 12: haven’t look into detail where the infinite loop occurs, but it seems like a hammer fix for the issue. The example mentioned in the test ((a + b) => (a - (-b)) => (a + b) => etc) makes me thinks that one of the two forms should be canonical and InstCombine should not make one of these two transforms. What do you think?
>
> I chose this way as it should guarantee to work for all cases, but you
> might be right in that something is wrong with pattern matching in
> InstCombine, see below.
>
> There is an implicit four step loop between InstCombiner::visitFAdd() and
> InstCombiner::visitFSub():
>
> 1. FAdd: Convert A to -A.
> 2. FSub: Convert -A to A; switch operands.
> 3. FAdd: Convert B to -B.
> 4. FSub: Convert -B to B; switch operands.
> 5. Go to step 1.
>
> Both functions try to do something about negative values, but they
> actually hang in case of positive arguments as well. I couldn't find
> any checks whether operands are negative, it seems to be "assumed".
> Maybe dyn_castFNegVal() used to perform the check, but it doesn't anymore.
>
> If I add `C->isNegative()` check to dyn_castFNegVal(), the issue
> disappears, but I'm not sure if it's the right place. Meaning of the
> comment there:
>
> // Constants can be considered to be negated values if they can be
> // folded.
> if (ConstantFP *C = dyn_cast<ConstantFP>(V))
> return ConstantExpr::getFNeg(C);
>
> is unclear to me. Does it suggest that the function doesn't care about
> sign of the value? In this case, checks for negative operands should be
> performed outside (in visitFAdd() and visitFSub() functions).
>
> Updated patches are attached (0000-recent-changes.diff contains latest
> changes only). There is nothing for InstCombine yet and a small new
> change (not counting test file) for LICM pass.
>
> Thanks,
> Sergey
-------------- next part --------------
commit 17e0043ca9b7da95a42ee5df868a37d6a19bab14
Author: Sergey Dmitrouk <sdmitrouk at accesssoftek.com>
Date: Fri Dec 19 15:08:59 2014 +0200
Prevent InstCombine from hanging
InstCombine shouldn't hang in non-terminating sequence of conversions like:
(a + b) => (a - (-b)) => (a + b) => etc.
---
lib/Transforms/InstCombine/InstCombineAddSub.cpp | 27 +++++++-------
test/Other/fpenv-constant-fold.ll | 9 ++---
.../do-not-hand-on-fpops-with-fpenv-access.ll | 42 ++++++++++++++++++++++
3 files changed, 62 insertions(+), 16 deletions(-)
diff --git a/lib/Transforms/InstCombine/InstCombineAddSub.cpp b/lib/Transforms/InstCombine/InstCombineAddSub.cpp
index 752f79d..8ce53d8 100644
--- a/lib/Transforms/InstCombine/InstCombineAddSub.cpp
+++ b/lib/Transforms/InstCombine/InstCombineAddSub.cpp
@@ -1342,22 +1342,25 @@ Instruction *InstCombiner::visitFAdd(BinaryOperator &I) {
return NV;
}
- // -A + B --> B - A
- // -A + -B --> -(A + B)
- if (Value *LHSV = dyn_castFNegVal(LHS)) {
- Instruction *RI = BinaryOperator::CreateFSub(RHS, LHSV);
- RI->copyFastMathFlags(&I);
- return RI;
- }
-
- // A + -B --> A - B
- if (!isa<Constant>(RHS))
- if (Value *V = dyn_castFNegVal(RHS)) {
- Instruction *RI = BinaryOperator::CreateFSub(LHS, V);
+ // Skip this to break implicit infinite loop with "fsub" transformations.
+ if (!I.hasKeepExceptions() && !I.hasKeepRounding()) {
+ // -A + B --> B - A
+ // -A + -B --> -(A + B)
+ if (Value *LHSV = dyn_castFNegVal(LHS)) {
+ Instruction *RI = BinaryOperator::CreateFSub(RHS, LHSV);
RI->copyFastMathFlags(&I);
return RI;
}
+ // A + -B --> A - B
+ if (!isa<Constant>(RHS))
+ if (Value *V = dyn_castFNegVal(RHS)) {
+ Instruction *RI = BinaryOperator::CreateFSub(LHS, V);
+ RI->copyFastMathFlags(&I);
+ return RI;
+ }
+ }
+
// Check for (fadd double (sitofp x), y), see if we can merge this into an
// integer add followed by a promotion.
if (SIToFPInst *LHSConv = dyn_cast<SIToFPInst>(LHS)) {
diff --git a/test/Other/fpenv-constant-fold.ll b/test/Other/fpenv-constant-fold.ll
index 01dbfa0..68b94ca 100644
--- a/test/Other/fpenv-constant-fold.ll
+++ b/test/Other/fpenv-constant-fold.ll
@@ -1,4 +1,5 @@
-; InstCombine is used just for its calls to constant folder.
+; InstCombine is used just for its calls to constant folder, it is the cause of
+; fsub -> fadd transformation.
; RUN: opt -S -instcombine -o - < %s | FileCheck %s
; Target independent constant folder should not fold floating-point operations
@@ -15,7 +16,7 @@ entry:
define double @do-not-fold-fsub-that-can-trap() {
; CHECK-LABEL: @do-not-fold-fsub-that-can-trap
-; CHECK: fsub
+; CHECK: fadd
entry:
%val = fsub kexc double 1.000000e-308, 1.000000e+308
ret double %val
@@ -55,7 +56,7 @@ entry:
define double @do-not-fold-fsub-because-of-rounding() {
; CHECK-LABEL: @do-not-fold-fsub-because-of-rounding
-; CHECK: fsub
+; CHECK: fadd
entry:
%val = fsub kround double 0.101, 0.93
ret double %val
@@ -92,7 +93,7 @@ define double @fold-fsub-that-cant-trap() {
; CHECK-LABEL: @fold-fsub-that-cant-trap
; CHECK-NOT: fsub
entry:
- %val = fsub kexc kround double 1.0, 1.0
+ %val = fadd kexc kround double 1.0, 1.0
ret double %val
}
diff --git a/test/Transforms/InstCombine/do-not-hand-on-fpops-with-fpenv-access.ll b/test/Transforms/InstCombine/do-not-hand-on-fpops-with-fpenv-access.ll
new file mode 100644
index 0000000..8ea33f0
--- /dev/null
+++ b/test/Transforms/InstCombine/do-not-hand-on-fpops-with-fpenv-access.ll
@@ -0,0 +1,42 @@
+; RUN: opt < %s -instcombine -S
+
+; InstCombine shouldn't hang in non-terminating sequence of conversions like:
+; (a + b) => (a - (-b)) => (a + b) => etc.
+
+define double @test1() {
+entry:
+ %add = fadd kexc double 0x3FF921FB54442D18, 0x3870000000000000
+ ret double %add
+}
+
+define double @test2(double %x) {
+entry:
+ %sub = fsub kexc kround double -0.000000e+00, 1.000000e+00
+ %div = fdiv kexc kround double %sub, %x
+ ret double %div
+}
+
+define double @test3() {
+entry:
+ %sub = fsub kexc kround double -0.000000e+00, 0x400921FB54442D18
+ ret double %sub
+}
+
+define double @test4(i32 %y) {
+entry:
+ %tobool = icmp ne i32 %y, 0
+ br i1 %tobool, label %cond.true, label %cond.false
+
+cond.true:
+ %sub = fsub kexc kround double -0.000000e+00, 0x400921FB54442D18
+ %div = fdiv kexc kround double %sub, 2.000000e+00
+ br label %cond.end
+
+cond.false:
+ %div1 = fdiv kexc kround double 0x400921FB54442D18, 2.000000e+00
+ br label %cond.end
+
+cond.end:
+ %cond = phi double [ %div, %cond.true ], [ %div1, %cond.false ]
+ ret double %cond
+}
More information about the llvm-commits
mailing list