<div dir="ltr"><div>A potential solution for this problem that would be more flexible and might *improve* compile-time: extend simplifyICmpWithMinMax() with the flexibility to create new instructions, and move it from InstSimplify to InstCombine.<br><br></div>I had been operating under the assumption that folds that qualify as a 'simplify' always belong in InstSimplify, but there are reasons to keep these in InstCombine instead as I learned with:<br><a href="https://reviews.llvm.org/D28337" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D28337</a><br><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jan 16, 2017 at 9:11 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 class="">On Mon, Jan 16, 2017 at 6:07 AM, Volkan Keles <span dir="ltr"><<a href="mailto:vkeles@apple.com" target="_blank">vkeles@apple.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><br><div><span class="m_4194514624505439375gmail-"><blockquote type="cite"><div>On Jan 13, 2017, at 3:16 PM, Sanjay Patel <<a href="mailto:spatel@rotateright.com" target="_blank">spatel@rotateright.com</a>> wrote:</div><br class="m_4194514624505439375gmail-m_-1368207291006311791Apple-interchange-newline"><div><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jan 13, 2017 at 5:24 AM, Volkan Keles <span dir="ltr"><<a href="mailto:vkeles@apple.com" target="_blank">vkeles@apple.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><br><div><blockquote type="cite"><span><div>On Jan 12, 2017, at 4:35 PM, Sanjay Patel <<a href="mailto:spatel@rotateright.com" target="_blank">spatel@rotateright.com</a>> wrote:</div><br class="m_4194514624505439375gmail-m_-1368207291006311791gmail-m_2307541518420224187m_-2570252747655182579Apple-interchange-newline"></span><div><div dir="ltr"><div><div><div>Hi Volkan,<br><br></div><span>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></span></div><span>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><blockquote type="cite"><div><div dir="ltr">2. You are opposed to the min/max canonicalization in this commit because it is increasing compile-time?</div></div></blockquote><div><br></div></span><div>Sorry, my e-mail was not clear. I meant the former, I wanted to get your opinion.</div></div></div></blockquote><div><br><br></div><div>I understand the concern, but this commit (like the min/max commit - <a href="https://reviews.llvm.org/D27531" target="_blank">https://reviews.llvm.org/D2753<wbr>1</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><br>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><br>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></div></span><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><span class="m_4194514624505439375gmail-"><br></span></div></div></blockquote><div><br></div></span><div>Sorry - I misunderstood your earlier answer. So you'd like to see this fold added to InstCombine because your pipeline doesn't include SCCP and you're not too concerned about compile-time. Can you file a bug report for this case, so we can reference it?<br></div><div><br></div><div>I'm not sure how others will react. They might ask: "Why not run SCCP since compile-time isn't a problem?" <br><br></div><div>FWIW, I started looking at InstCombine folds involving phi nodes with:<br><a href="https://reviews.llvm.org/D28625" target="_blank">https://reviews.llvm.org/<wbr>D28625</a><br></div><div>...I haven't tried many loop optimizations before, so I'm still learning about safely handling those. The logic and pattern-matching to make the transform that you want for this fold is hopefully similar: we need to thread the phi's incoming values into the users and evaluate them.<br></div><span class=""><div><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><div><span class="m_4194514624505439375gmail-"><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>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></div></div></div></div></div></blockquote><div><br></div></span><div>No, it doesn’t.</div><span class="m_4194514624505439375gmail-"><br><blockquote type="cite"><div><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><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><div><span><blockquote type="cite"><div><div dir="ltr"><div><div><div><div><div class="gmail_extra"><br></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></div></div></div></div></div></div></div></blockquote><div><br></div></span>Yes, I agree.</div><div><br></div><div><span><blockquote type="cite"><div><div dir="ltr"><div><div><div><div><div class="gmail_extra"><br></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></div></div></div></div></div></div></div></blockquote></span><br></div></div></blockquote></div></div></div>
</div></blockquote></span></div><span class="m_4194514624505439375gmail-HOEnZb"><font color="#888888"><br><div>Volkan</div></font></span></div></blockquote></span></div><br></div></div>
</blockquote></div><br></div>