This is an archive of the discontinued LLVM Phabricator instance.

[InstSimplify] Match 1.0 and 0.0 for both operands in SimplifyFMAMul
ClosedPublic

Authored by fhahn on Sep 13 2019, 7:34 AM.

Details

Summary

Because we do not constant fold multiplications in SimplifyFMAMul,
we match 1.0 and 0.0 for both operands, as multiplying by them
is guaranteed to produce an exact result (if it is allowed to do so).

Note that it is not enough to just swap the operands to ensure a
constant is on the RHS, as we want to also cover the case with
2 constants.

Event Timeline

fhahn created this revision.Sep 13 2019, 7:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 13 2019, 7:34 AM
Herald added a subscriber: hiraditya. · View Herald Transcript

LG

llvm/lib/Analysis/InstructionSimplify.cpp
4561

I'm wondering if

{
if(isa<Constant>(Op0)) std::swap(Op0, Op1);

would be simpler?

fhahn marked an inline comment as done.Sep 13 2019, 7:46 AM
fhahn added inline comments.
llvm/lib/Analysis/InstructionSimplify.cpp
4561

I initially also used to swap the operands, but we'd also have to check if it's a 1.1 or 0.0 constant I think, to handle fmul 10.0, 0.0. That's not a problem with SimplifyFMulInst, as constant folding would cover this case.

reames accepted this revision.Sep 13 2019, 9:16 AM
reames added inline comments.
llvm/lib/Analysis/InstructionSimplify.cpp
4561

I believe that fmul is not necessarily commutative (for nan inputs). Given that, swap would be correct for this case, but dangerous in general.

This revision is now accepted and ready to land.Sep 13 2019, 9:16 AM
reames added inline comments.Sep 13 2019, 9:17 AM
llvm/lib/Analysis/InstructionSimplify.cpp
4561

Er, maybe I'm wrong here. At least the accessors in Instruction.h seems to think so.

lebedev.ri accepted this revision.Sep 13 2019, 9:27 AM

I'm fine with this approach.

llvm/lib/Analysis/InstructionSimplify.cpp
4561

I'm pretty sure fadd/fmul are commutative (X * Y == Y * X), but not associative ((X * Y) * Z != (X * Z) * Y).
So accessors are likely wrong. Or many more code is wrong elsewhere.

This revision was automatically updated to reflect the committed changes.