<div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr">On Tue, Sep 25, 2018 at 7:37 PM Sanjay Patel <<a href="mailto:spatel@rotateright.com">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"><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-width:1px;border-left-style:solid;border-left-color: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-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></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.</div></div></div></div></div></div></blockquote><div><br></div><div><div>Oh, I see. So you're worried that an FNeg won't match the generic m_c_BinOp(...) as it does now. That's a reasonable concern.</div><div><br></div><div>This is the first time I'm looking at foldShuffledBinop(...), so maybe a naive question, but why not do similar shuffle canonicalizations on unary (or ternary) operations? That may be a better fix in the long run.</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 dir="ltr"><div dir="ltr"><div dir="ltr"><div class="gmail_quote"><div></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.</div></div></div></div></div></div></blockquote><div><br></div><div>Ah, yeah. So this worries me too. Both floating point and integer negate are syntactic sugar for (-0-x). I did not know that. It would be ugly to unwrap floating point negate without doing the same for integer negate. :/</div><div><br></div></div></div></div></div></div>