[PATCH] Flag to enable IEEE-754 friendly FP optimizations

Sergey Dmitrouk sdmitrouk at accesssoftek.com
Thu Feb 19 07:34:26 PST 2015


Ping.

On Tue, Feb 10, 2015 at 08:12:13PM +0200, Sergey Dmitrouk wrote:
> 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
> 





More information about the llvm-commits mailing list