<div dir="ltr"><div><div>[Using new mailing list. Several of the previous mails in this thread are probably lost forever for the lists; see below for context.]<br><br></div>I don't think we should leave CSE in the DAG broken while figuring out how to solve this, so I reverted r243687 with:<br><a href="http://llvm.org/viewvc/llvm-project?view=revision&revision=244053" target="_blank">http://llvm.org/viewvc/llvm-project?view=revision&revision=244053</a><br><br></div><div>Also filed a bug using the example code below:<br><a href="https://llvm.org/bugs/show_bug.cgi?id=24363">https://llvm.org/bugs/show_bug.cgi?id=24363</a><br><br></div><div>So we may get at least one in-tree regression test before the flag gets enabled again. :)<br></div><br><div><div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 4, 2015 at 5:39 PM, Sanjay Patel <span dir="ltr"><<a href="mailto:spatel@rotateright.com" target="_blank">spatel@rotateright.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Thanks - your example made it much clearer to me how this goes wrong. I think I should revert r243687 and try to fix everything under the cover of that scaffolding...<br><br>I don't know how we'd test it all though. Here's an example that shows the bug for one spot in DAGCombiner's GetNegatedExpression() with an x86 target:<br><br>$ cat badflags.ll <br>define float @fmf(float %a, float %b) {<br> %mul1 = fmul fast float %a, %b<br> %nega = fsub fast float 0.0, %a<br> %mul2 = fmul fast float %nega, %b<br> %abx2 = fsub fast float %mul1, %mul2<br> ret float %abx2<br>}<br><br><br>$ ./llc -o - badflags.ll -mattr=fma -enable-unsafe-fp-math -enable-fmf-dag=0 <br>...<br> vmulss %xmm1, %xmm0, %xmm0<br> vaddss %xmm0, %xmm0, %xmm0<br> retq<br><br>$ ./llc -o - badflags.ll -mattr=fma -enable-unsafe-fp-math -enable-fmf-dag=1<br>...<br> vmulss %xmm1, %xmm0, %xmm2<br> vfmadd213ss %xmm2, %xmm1, %xmm0 <--- failed to recognize that (a * b) was already calculated<br> retq<br><br></div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 4, 2015 at 12:03 PM, escha <span dir="ltr"><<a href="mailto:escha@apple.com" target="_blank">escha@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Unfortunately, no :/ though I can imagine one could be constructed in *some* fashion, since surely crippled CSE is visible somehow somewhere. The particular case that came up looked like this:<div><br></div><div>c = a * b FAST</div><div>d = -c FAST</div><div><br></div><div>(combine1 occurs)</div><div><br></div><div>c = a * b FAST</div><div>d = a * -b</div><div><br></div><div>then we have a combine in combine2, in our target, to deduplicate these, which should result in this:</div><div><br></div><div><div>c = a * b FAST</div><div>d = -c FAST</div></div><div><br></div><div>which we do because negates are free (modifiers on a GPU) while an extra multiply is not.</div><div><br></div><div>it visits d = a * -b and then searches the DAG to see if (a * b) exists. But it doesn’t, only (a * b FAST) exists, the search fails, and the code is pessimized.</div><span><font color="#888888"><div><br></div><div>—escha</div></font></span><div><div><div><br><div><blockquote type="cite"><div>On Aug 4, 2015, at 10:57 AM, Sanjay Patel <<a href="mailto:spatel@rotateright.com" target="_blank">spatel@rotateright.com</a>> wrote:</div><br><div><div dir="ltr">Thanks! Do you have a test case that would show a difference from that kind of change on an in-tree backend? <br><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 4, 2015 at 11:34 AM, escha <span dir="ltr"><<a href="mailto:escha@apple.com" target="_blank">escha@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">I’ve only done the following patch, just as an experiment; I haven’t done much work on it overall (I was just experimenting with trying to kill off this individual regression).<div><br></div><div></div></div><br><div style="word-wrap:break-word"><div><br><div><br></div><div>—escha</div><div><br><div><blockquote type="cite"><div>On Aug 4, 2015, at 10:07 AM, Sanjay Patel <<a href="mailto:spatel@rotateright.com" target="_blank">spatel@rotateright.com</a>> wrote:</div><br><div><div dir="ltr"><div>Ok, that's good news. The other good news is that nobody has reported crashes from this change...yet.<br><br></div>I'm fine proceeding either way (revert r243687 or just start patching as-is). Let me know what you'd prefer.<br><br>Can you commit any patches you've already done for the generic DAG classes?<br><div><div><div><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 4, 2015 at 10:47 AM, escha <span dir="ltr"><<a href="mailto:escha@apple.com" target="_blank">escha@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">I have a patch to add propagation to our own backend, by the way; it wasn’t at all difficult to write (it took like 20 minutes). But if we keep the patch in, we should probably put out an announcement to backend maintainers that they need to add fast-math flag propagation.<span><font color="#888888"><div><br></div><div>—escha</div></font></span><div><div><div><br><div><blockquote type="cite"><div>On Aug 4, 2015, at 8:28 AM, escha <<a href="mailto:escha@apple.com" target="_blank">escha@apple.com</a>> wrote:</div><br><div><div style="word-wrap:break-word"><div>The “unsafe FP math” transforms aren’t enough; the problem is that even the *safe-math* transforms need to propagate flags from their source nodes, or you end up with un-CSEable duplicates (and of course, lose flags for future transformations!). You need to pass flags in for *every single creation of an FADD, FSUB, FADD, or FDIV*, as far as I can tell. This involves a huge amount of diffs that look like this:</div><div><br></div><div><div style="margin:0px;font-size:11px;font-family:Menlo"> // fold (fneg (fsub A, B)) -> (fsub B, A)</div><div style="margin:0px;font-size:11px;font-family:Menlo"> return DAG.getNode(ISD::FSUB, SDLoc(Op), Op.getValueType(),</div><div style="margin:0px;font-size:11px;font-family:Menlo;color:rgb(195,55,32)">- Op.getOperand(1), Op.getOperand(0));</div><div style="margin:0px;font-size:11px;font-family:Menlo;color:rgb(52,189,38)">+ Op.getOperand(1), Op.getOperand(0),</div><div style="margin:0px;font-size:11px;font-family:Menlo;color:rgb(52,189,38)">+ &cast<BinaryWithFlagsSDNode>(Op)->Flags);</div></div><div><br></div><div>One idea I had was to allow conservative CSE. That is, allow CSEing nodes with fast-math flags, even if their flags differ — just, when CSEing them, pick the intersection of the two flags. This will guarantee that CSE will still function, even if DAG folds don’t properly maintain fast-math flags. But we still need to eventually add all these flag propagations; otherwise having fast-math flags in the DAG is useless at best, isn’t it?</div><div><br></div><div>—escha</div><div><br></div><div><br></div><div><br><div><blockquote type="cite"><div>On Aug 4, 2015, at 7:26 AM, Sanjay Patel <<a href="mailto:spatel@rotateright.com" target="_blank">spatel@rotateright.com</a>> wrote:</div><br><div><div dir="ltr"><div><div>Does anyone see a workaround to do this piecemeal with FMF enabled?<br><br></div>Otherwise, I see about 30 checks for UnsafeFPMath transforms in DAGCombiner...so I could revert r243687, start working on the combiner fixes, and then turn the flag back on when DAGCombiner is FMF-clean.<br><br>But this wouldn't be perfect if you have more UnsafeFPMath transforms in your backend. You'd have to patch those too before we turn the flag back on.<br><br></div>Any other ideas about how to proceed?<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Aug 3, 2015 at 4:52 PM, escha <span dir="ltr"><<a href="mailto:escha@apple.com" target="_blank">escha@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">We’re getting significant CSE regressions on our backend, caused by the fact that none of the DAG folds propagate fast-math flags. This results in nodes not being CSE’d, since we only CSE nodes if they have identical flags (and thus worse codegen).<div><br></div><div>I was able to get rid of many of them by adding flag propagation to FNEG folding, but there’s way more than just that.<span><font color="#888888"><br></font></span><div><span><font color="#888888"><div><br></div><div>—escha</div></font></span><div><br><div><blockquote type="cite"><div><div><div>On Jul 29, 2015, at 11:51 AM, Sanjay Patel <<a href="mailto:spatel@rotateright.com" target="_blank">spatel@rotateright.com</a>> wrote:</div><br></div></div><div><div><div><div dir="ltr">For reference, fixes for PR24141 are in:<br><pre><a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_rL243293&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=EH36SVVrOANvPpWUfmtjHvcZene16JZYtYcOsOWHmmQ&s=oJkUxyP-U28dL9zPjcyCjESFgqf9fEa1WxScYHC1BLE&e=" target="_blank">http://reviews.llvm.org/rL243293</a>
<a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_rL243498&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=EH36SVVrOANvPpWUfmtjHvcZene16JZYtYcOsOWHmmQ&s=9UShb3DxctMQors5uP4ErkfZ2ETsaqYXEeRLX8qKYt0&e=" target="_blank">http://reviews.llvm.org/rL243498</a>
<a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_rL243500&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=EH36SVVrOANvPpWUfmtjHvcZene16JZYtYcOsOWHmmQ&s=S5dEJW-NYcq9yLglX74iN2esNypm3qfP6wCNidxysx8&e=" target="_blank">http://reviews.llvm.org/rL243500</a></pre>...assuming those don't cause new breakage, it's finally, really time to flip the FMF flag...and wait for more breakage.<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jul 20, 2015 at 11:34 AM, Sanjay Patel <span dir="ltr"><<a href="mailto:spatel@rotateright.com" target="_blank">spatel@rotateright.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Fri, Jul 17, 2015 at 4:42 PM, Nick Lewycky <span dir="ltr"><<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>></span> wrote:<br><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 class="gmail_extra"><div class="gmail_quote"><span>On 9 July 2015 at 15:27, Sanjay Patel <span dir="ltr"><<a href="mailto:spatel@rotateright.com" target="_blank">spatel@rotateright.com</a>></span> wrote:<br><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>Hi Nick,<br><br></div>The reciprocal fix is in at r241826. Please let me know when you find another failure. Thanks!<br></div></blockquote><div><br></div></span><div>Thank you! I tested it and found exactly nothing wrong. Hopefully I didn't goof the testing, but I think it's time to flip the flag to true, then wait a few days before removing the flag entirely.<br></div></div></div></div></blockquote><div><br></div></span><div>Wow...I'm surprised, but I'll hope for the best. Thanks for the testing!<br></div><span><div><br><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 class="gmail_extra"><div class="gmail_quote"><div></div><div>Note however, that we still have problems with PR24141 which is related to unsafe fp math, but doesn't change with the -enable-fmf-dag flag.</div></div></div></div></blockquote></span><div><br>Proposed fix:<br><a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D11345&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=EH36SVVrOANvPpWUfmtjHvcZene16JZYtYcOsOWHmmQ&s=q0g5ELtAiUzID-3xj21ZzkiASkbrFaYCWXDy-nBF0OI&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/D11345</a><br> </div></div><br></div></div>
</blockquote></div><br></div></div></div><span>
_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br></span></div></blockquote></div><br></div></div></div></div></blockquote></div><br></div>
</div></blockquote></div><br></div></div>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br></div></blockquote></div><br></div></div></div></div></blockquote></div><br></div></div></div></div></div>
</div></blockquote></div><br></div></div></div><br></blockquote></div><br></div>
</div></blockquote></div><br></div></div></div></div></blockquote></div><br></div>
</div></div></blockquote></div><br></div></div></div></div></div></div>