+1 for an explicit FNEG instruction, since as previously discussed, it has stricter requirements for what value may be returned by the operation. And strengthening the requirement on FSUB is not feasible when the values are variables rather than literals. That is: FSUB(-0.0, NaN) = either NaN *or* -NaN FSUB(-0.0, -NaN) = either NaN *or* -NaN FNEG(NaN) = -NaN FNEG(-NaN) = NaN On Tue, Sep 11, 2018 at 3:35 PM Cameron McInally via llvm-dev < llvm-dev at lists.llvm.org> wrote:> On Tue, Sep 11, 2018 at 2:45 PM, Kevin Neal via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > >> Which exactly was the plan? >> >> >> >> Add a new, regular instruction? >> >> >> >> Add a new constrained math intrinsic? >> >> >> >> Both? >> >> >> > > I'd like to add an explicit FNEG IR operator. We would not need a > constrained FNEG operator if we go this route. > > >> Andrew Kaylor made a good point here: >> >> - As I said, all LLVM IR FP instructions are //assumed// to have no >> side effects. I'm not sure we want an instruction that goes beyond this to >> be //defined// as having no side effects. It adds a complication to the >> language and introduces restrictions on the code generator that aren't >> needed in the ordinary case of non-constrained FP. The target code >> generators are free to do anything they want with the other FP >> instructions, including things that introduce new FP status flags being set >> that otherwise wouldn't be, and for the normal case the back ends should be >> free to do that with fneg as well. >> >> >> >> Personally, I’m not sure I like the idea of having exceptions to the rule >> that FP instructions also have constrained versions. So I lean towards >> having both a regular FNEG and a constrained version. >> >> >> >> But I think I remember pushback. I can’t put my fingers on the message, >> though. >> > > We touched on this in the Differential Review and on this thread. To > summarize: > > FNEG(X) is not really the same operation as FSUB(-0.0, X), although the > differences are admittedly subtle. I even went as far to say that any > xforms between the two operations should only occur under FastMath > conditions. If we follow those rules, I think emergence guarantees that we > don't have to worry about the side effects of FNEG (please correct me if > I've missed something). > > Extending on that, I suspect that we should not be canonicalizing FNEG(X) > as FSUB(-0.0, X), but rather as XOR(X, 1 << (SIZE_OF_TYPE_IN_BITS - 1)). A > utility function could provide us with the sign-bit constant, so it's not > that ugly. > > That said, I agree that Andrew's take is compelling. And I would be okay > with adding a constrained FNEG to solve the immediate issue, if that is the > final decision. > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20180911/2d92395f/attachment.html>
I have 1 concern about adding an explicit fneg op to IR: Currently, fneg qualifies as a binop in IR (since there's no other way to represent it), and we have IR transforms that are based on matching that pattern (m_BinOp). With a proper unary fneg instruction, those transforms are not going to fire anymore. So I think we'll need to duplicate code to account for that difference (or hack the matcher to include fneg?). The existing regression tests probably aren't going to show optimization failures for those cases*, so I'm not sure how to detect regressions without doing an audit of FP transforms in instcombine and other passes. * Are we proposing to canonicalize "fsub -0.0, X --> fneg X"? As shown in the example below, I think that's allowed. If we do that, then we might detect some of the affected transforms, but I suspect that we don't have that test coverage for most transforms. On Tue, Sep 11, 2018 at 1:46 PM James Y Knight via llvm-dev < llvm-dev at lists.llvm.org> wrote:> +1 for an explicit FNEG instruction, since as previously discussed, it has > stricter requirements for what value may be returned by the operation. And > strengthening the requirement on FSUB is not feasible when the values are > variables rather than literals. > > That is: > FSUB(-0.0, NaN) = either NaN *or* -NaN > FSUB(-0.0, -NaN) = either NaN *or* -NaN > > FNEG(NaN) = -NaN > FNEG(-NaN) = NaN > > > On Tue, Sep 11, 2018 at 3:35 PM Cameron McInally via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > >> On Tue, Sep 11, 2018 at 2:45 PM, Kevin Neal via llvm-dev < >> llvm-dev at lists.llvm.org> wrote: >> >>> Which exactly was the plan? >>> >>> >>> >>> Add a new, regular instruction? >>> >>> >>> >>> Add a new constrained math intrinsic? >>> >>> >>> >>> Both? >>> >>> >>> >> >> I'd like to add an explicit FNEG IR operator. We would not need a >> constrained FNEG operator if we go this route. >> >> >>> Andrew Kaylor made a good point here: >>> >>> - As I said, all LLVM IR FP instructions are //assumed// to have no >>> side effects. I'm not sure we want an instruction that goes beyond this to >>> be //defined// as having no side effects. It adds a complication to the >>> language and introduces restrictions on the code generator that aren't >>> needed in the ordinary case of non-constrained FP. The target code >>> generators are free to do anything they want with the other FP >>> instructions, including things that introduce new FP status flags being set >>> that otherwise wouldn't be, and for the normal case the back ends should be >>> free to do that with fneg as well. >>> >>> >>> >>> Personally, I’m not sure I like the idea of having exceptions to the >>> rule that FP instructions also have constrained versions. So I lean towards >>> having both a regular FNEG and a constrained version. >>> >>> >>> >>> But I think I remember pushback. I can’t put my fingers on the message, >>> though. >>> >> >> We touched on this in the Differential Review and on this thread. To >> summarize: >> >> FNEG(X) is not really the same operation as FSUB(-0.0, X), although the >> differences are admittedly subtle. I even went as far to say that any >> xforms between the two operations should only occur under FastMath >> conditions. If we follow those rules, I think emergence guarantees that we >> don't have to worry about the side effects of FNEG (please correct me if >> I've missed something). >> >> Extending on that, I suspect that we should not be canonicalizing FNEG(X) >> as FSUB(-0.0, X), but rather as XOR(X, 1 << (SIZE_OF_TYPE_IN_BITS - 1)). A >> utility function could provide us with the sign-bit constant, so it's not >> that ugly. >> >> That said, I agree that Andrew's take is compelling. And I would be okay >> with adding a constrained FNEG to solve the immediate issue, if that is the >> final decision. >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20180925/e285fda5/attachment.html>
On Tue, Sep 25, 2018 at 1:39 PM Sanjay Patel <spatel at rotateright.com> wrote:> I have 1 concern about adding an explicit fneg op to IR: > > Currently, fneg qualifies as a binop in IR (since there's no other way to > represent it), and we have IR transforms that are based on matching that > pattern (m_BinOp). With a proper unary fneg instruction, those transforms > are not going to fire anymore. So I think we'll need to duplicate code to > account for that difference (or hack the matcher to include fneg?). >>The existing regression tests probably aren't going to show optimization> failures for those cases*, so I'm not sure how to detect regressions > without doing an audit of FP transforms in instcombine and other passes. >Implementation details. ¯\_(ツ)_/¯ Seriously though, this is interesting and something that I had not considered. Thinking aloud, what if we caught the FNEG pattern in the CreateFSub*(...) IRBuilder functions and generated an FNeg instruction instead? That way we could get rid of the m_FNeg(...) calls altogether. They would just be replaced with something like: (I.getOpcode() =Instruction::FNeg). Any transform that currently uses m_FNeg(...) would fail to build without an update. But maybe I'm not considering some substitution that could modify an FSub instruction in place??> * Are we proposing to canonicalize "fsub -0.0, X --> fneg X"? As shown in > the example below, I think that's allowed. If we do that, then we might > detect some of the affected transforms, but I suspect that we don't have > that test coverage for most transforms. >It sounds reasonable to continue c14n on "fsub -0.0, X --> fneg X". The problematic case is "fneg X --> fsub -0.0, X". Although, that does raise the question of who defines undefined behavior: LLVM or the hardware? I would lean towards falling back to the hardware behavior. Take integer divide by zero for example. I'd much rather the hardware define i/0 (e.g. x86 will trap) than LLVM. -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20180925/a4adb3f9/attachment.html>