
<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><br class=""><div><blockquote type="cite" class=""><div class="">On Jan 13, 2017, at 3:16 PM, Sanjay Patel <<a href="mailto:spatel@rotateright.com" class="">spatel@rotateright.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div dir="ltr" class=""><br class=""><div class="gmail_extra"><br class=""><div class="gmail_quote">On Fri, Jan 13, 2017 at 5:24 AM, Volkan Keles <span dir="ltr" class=""><<a href="mailto:vkeles@apple.com" target="_blank" class="">vkeles@apple.com</a>></span> wrote:<br class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;" class=""><br class=""><div class=""><blockquote type="cite" class=""><span class=""><div class="">On Jan 12, 2017, at 4:35 PM, Sanjay Patel <<a href="mailto:spatel@rotateright.com" target="_blank" class="">spatel@rotateright.com</a>> wrote:</div><br class="gmail-m_2307541518420224187m_-2570252747655182579Apple-interchange-newline"></span><div class=""><div dir="ltr" class=""><div class=""><div class=""><div class="">Hi Volkan,<br class=""><br class=""></div><span class="">I reduced your test case and explained what I found below. It's not clear to me what you mean by linking the compile-time thread:<br class=""></span></div><span class="">1. SCCP handles this case already, so we shouldn't need to add a duplicate fold to InstCombine and increase compile-time further?</span></div></div></div></blockquote><span class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class="">2. You are opposed to the min/max canonicalization in this commit because it is increasing compile-time?</div></div></blockquote><div class=""><br class=""></div></span><div class="">Sorry, my e-mail was not clear. I meant the former, I wanted to get your opinion.</div></div></div></blockquote><div class=""><br class=""><br class=""></div><div class="">I understand the concern, but this commit (like the min/max commit - <a href="https://reviews.llvm.org/D27531" class="">https://reviews.llvm.org/D27531</a> - that was inspired by this thread and referenced in the compile-time thread) is a very simple matcher. If these kinds of commits are causing noticeable slowdowns, then we need a structural fix...or LLVM is doing a lousy job of optimizing its own code - PR28430 is a bit embarrassing.<br class=""><br class="">IMO, we need many more simple canonicalizations like this to make life easier for the backend. I've explained the rationale for the min/max additions (better codegen). This actually goes back to an earlier, more general suggestion on llvm-dev that we should canonicalize harder towards 'select' instructions in IR. We may want to rethink that strategy given the compile-time creep, but I'm definitely not knowledgeable enough to make that decision.<br class=""><br class="">Several suggestions were made in the compile-time thread about improving instcombine, so I think that's where we should continue the discussion about compile-time in general.</div></div></div></div></div></blockquote><div><br class=""></div><div>I’m not opposed to the min/max canonicalization and I’m not worried about its impact on compile-time, it was related to InstCombine. I wanted to know if that expensive call was going to be added to InstCombine. </div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""><br class=""></div><div class="">I am curious about something here though. You noticed this difference in instsimplify behavior - does that mean that SCCP doesn't run in your optimization pipeline?<br class=""></div></div></div></div></div></blockquote><div><br class=""></div><div>No, it doesn’t.</div><br class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class="gmail_extra"><div class="gmail_quote"><div class=""> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div style="overflow-wrap: break-word;" class=""><div class=""><span class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><div class=""><div class=""><div class=""><div class="gmail_extra"><br class=""></div><div class="gmail_extra">As I said earlier, the canonicalization seems equally likely to enable more folding in InstSimplify as it does to bypass it. Do you agree?<br class=""></div></div></div></div></div></div></div></blockquote><div class=""><br class=""></div></span>Yes, I agree.</div><div class=""><br class=""></div><div class=""><span class=""><blockquote type="cite" class=""><div class=""><div dir="ltr" class=""><div class=""><div class=""><div class=""><div class=""><div class="gmail_extra"><br class=""></div><div class="gmail_extra">My motivation for canonicalization of min/max patterns is that the backend is full of holes recognizing and lowering these patterns, so the more we can standardize them here in IR, the less we have to worry about botching the lowering. I have not attempted to weigh the compile-time cost of canonicalization against the potential runtime perf wins.<br class=""></div></div></div></div></div></div></div></blockquote></span><br class=""></div></div></blockquote></div></div></div>

</div></blockquote></div><br class=""><div class="">Volkan</div></body></html>