<div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr">On Tue, Sep 25, 2018 at 1:39 PM Sanjay Patel <<a href="mailto:spatel@rotateright.com" target="_blank">spatel@rotateright.com</a>> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">I have 1 concern about adding an explicit fneg op to IR:</div><div dir="ltr"><br> </div><div dir="ltr">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?). </div></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"> </div></div></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr">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.<br></div></div></blockquote><div><br></div><div><div>Implementation details. <span style="color:rgb(0,0,0);font-family:"helvetica neue",Helvetica,Arial,sans-serif;font-size:16px">¯\_(ツ)_/¯</span></div><div><span style="color:rgb(0,0,0);font-family:"helvetica neue",Helvetica,Arial,sans-serif;font-size:16px"><br></span></div><div>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.</div><div><br></div><div>But maybe I'm not considering some substitution that could modify an FSub instruction in place??</div></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div>* 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.</div></div></blockquote><div><br></div><div>It sounds reasonable to continue c14n on "fsub -0.0, X --> fneg X". The problematic case is "fneg X --> fsub -0.0, X". </div><div><br></div><div>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. </div></div></div></div></div></div></div></div>