<div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Tue, Sep 25, 2018 at 2:28 PM Cameron McInally <<a href="mailto:cameron.mcinally@nyu.edu" target="_blank">cameron.mcinally@nyu.edu</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><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:1px solid 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:1px solid 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:1px solid 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></div></div></div></div></div></div></div></blockquote><div><br></div><div>We don't want to get rid of the m_FNeg calls. Those are safe because they're providing an abstraction to the callers. Ie, anywhere we use m_FNeg() or Builder.CreateFNeg() should Just Work regardless of how we implement the underlying fneg operation. (We have to fix those function themselves of course to deal with the new opcode.) It's the code that doesn't use those abstractions that has the potential to regress.</div><div><br></div><div>Here's an example:</div><div><a href="https://reviews.llvm.org/rL343041" rel="noreferrer" target="_blank">https://reviews.llvm.org/rL343041</a></div><div><br></div><div>The transform in question is in InstCombiner::foldShuffledBinop(). It moves a binop ahead of a vector shuffle. It's called for every vector binop instruction, so if IR has a unary fneg and we want that to behave as it does today, we'd have to duplicate that fold for fneg (or fake it by substituting an fsub).<br></div><div><br></div><div>But after rummaging around in instcombine at least, I'm actually not too worried by the proposed change. Transforms on generic binops are not common.<br></div><div><br></div><div>But glancing at Reassociate.cpp is scarier. It does a lot of stuff like this:</div><div>    if (BinaryOperator::isNeg(TheOp) || BinaryOperator::isFNeg(TheOp))<br>      X = BinaryOperator::getNegArgument(TheOp);<br></div><div><br></div><div>I think that's going to fail without a (terrible) hack to treat the proposed fneg as a binop. So that's probably a preliminary requirement: find all uses of BinaryOperator::isFNeg() and update them to use m_FNeg(). That should be done ahead of the IR change to limit damage. I don't know if that's enough as-is, but we know that code will break without an update.<br></div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div class="gmail_quote"><div><div></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:1px solid 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>
</blockquote></div></div></div></div></div>