<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Apr 22, 2016 at 10:39 AM, Philip Reames <span dir="ltr"><<a href="mailto:listmail@philipreames.com" target="_blank">listmail@philipreames.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 text="#000000" bgcolor="#FFFFFF"><span class="">
    <br>
    <br>
    <div>On 04/22/2016 09:20 AM, Sanjay Patel
      via llvm-dev wrote:<br>
    </div>
    <blockquote type="cite">
      <div dir="ltr">
        <div>
          <div>I've proposed removing the llvm.expect intrinsic:<br>
            <a href="http://reviews.llvm.org/D19300" target="_blank">http://reviews.llvm.org/D19300</a><br>
            <br>
          </div>
          The motivation for this change is in:<br>
          <a href="http://reviews.llvm.org/D19299" target="_blank">http://reviews.llvm.org/D19299</a><br>
          <br>
          For reference:<br>
          1. We created an intrinsic that's only reason for existing is
          to improve perf, but the intrinsic can harm optimization by
          interfering with transforms in other passes.<br>
        </div>
      </div>
    </blockquote></span>
    I don't follow this at all.  Given expects are eagerly lowered to
    metadata, where's the interaction?  In particular, the expect
    lowering overrules any metadata on the associated branch/switch. 
    This is exactly what you'd expect for a user annotation interacting
    with PGO data.</div></blockquote><div><br></div><div>I agree that a user annotation should override PGO data. This is why I initially proposed that builtin_expect() would extend on '[un]predictable' metadata rather than 'prof' metadata  in D19299. <br><br>But I think that's a separate discussion now; we use 'prof' metadata for this purpose today, and I'm not trying to change that in these patches. If the user-specified representation of builtin_expect() is overwritten by PGO data, I think that's a bug independent of the current proposal/patches.<br></div><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 text="#000000" bgcolor="#FFFFFF"><span class=""><br>
    <blockquote type="cite"><br><div dir="ltr"><div>
          2. To solve that, we created a pass to always transform the
          intrinsic into metadata at a very early stage in LLVM. But now
          every program is paying a compile-time tax (running the
          LowerExpectIntrinsic pass) for a feature that is rarely used.<br>
        </div>
      </div>
    </blockquote></span>
    Er, what cost?  Given this is a single linear pass over the IR - and
    could actually be made essentially free by checking to see if the
    module has any uses of expect - I'm suspicious of this compile time
    argument.  Have you actually seen this in profiles?  <br></div></blockquote><div><br></div><div>No - I expect the actual overhead is in the noise. The real objection to the LowerExpectIntrinsic pass is that it is completely unnecessary. This was raised in the original review. We shouldn't have two mechanisms to represent exactly the same thing. This is also why my initial implementation for builtin_unpredictable() was rejected:<br><a href="http://reviews.llvm.org/D12341">http://reviews.llvm.org/D12341</a><br><br></div><div>I assumed there was some good reason for LowerExpectIntrinsic to exist, so I copied that design. As noted in that review, my assumption was wrong. And it was suggested then that we should remove LowerExpectIntrinsic but nobody had gotten around to it. With these patches, I'd like to finally fix this.<br></div><div><br><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 text="#000000" bgcolor="#FFFFFF"><span class="">
    <blockquote type="cite">
      <div dir="ltr">
        <div><br>
          A possible front-end replacement transformation for a
          source-level "builtin_expect()" is in D19299: I think a
          front-end can always directly create metadata for this kind of
          programmer hint rather than using an intermediate
          representation (the intrinsic). Likewise, if there's some
          out-of-tree IR pass that is creating an llvm.expect, it should
          be able to create branch weight metadata directly instead.<br>
        </div>
      </div>
    </blockquote></span>
    This seems like a reasonable proposal.  The expect intrinsic does
    give us a mechanism to express value profiling predictions, but we
    don't appear to actually use that today.  My bias would be to leave
    it in place, but I'm not going to object strongly if the consensus
    goes the other way.  <br>
    <blockquote type="cite"><span class="">
      <div dir="ltr">
        <div><br>
          Please let me know if you see any problems with this proposal
          or the patches.<br>
          <br>
        </div>
        <div>For reference, here's the original post-commit review
          thread for llvm.expect:<br>
          <a href="https://marc.info/?l=llvm-commits&m=130997676129580&w=4" rel="noreferrer" target="_blank">https://marc.info/?l=llvm-commits&m=130997676129580&w=4</a><br>
          <br>
        </div>
        <div><br>
        </div>
      </div>
      <br>
      <fieldset></fieldset>
      <br>
      </span><pre>_______________________________________________
LLVM Developers mailing list
<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev</a>
</pre>
    </blockquote>
    <br>
  </div>

</blockquote></div><br></div></div>