<div dir="ltr"><div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Tue, Sep 25, 2018 at 7:47 PM Cameron McInally <<a href="mailto:cameron.mcinally@nyu.edu">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"><br><div class="gmail_quote"><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></div></div></div></blockquote><div><br></div><div>AFAIK, all of the math/logic folding that we do currently is on binary operators because all of the instructions have that form:</div><div><a href="http://llvm.org/docs/LangRef.html#instruction-reference">http://llvm.org/docs/LangRef.html#instruction-reference</a><br></div><div><br></div><div>As discussed, we fake the unary neg/not/fneg as binops. Excluding control-flow, the only unary instructions are casts, and I don't see any ternary or higher math ops other than intrinsics.<br></div><div><br></div><div>I don't think there's any incentive to change the integer neg/not, so that makes fneg the oddball (and maybe that's why it was implemented as it is?).<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 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"><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></div></div></div></div></blockquote><div><br></div><div>Agreed - regardless of the outcome of fneg, it's probably worth replacing all of those fake binop API uses just to make the code uniform.<br></div></div></div></div>