<div dir="ltr">Hi Philip,<div><br></div><div>Sorry about the delayed reply.</div><div><br></div><div>I'm going to commit as is. In the future though, I think we could chose the code sequence based on the code-model. Small-code model would get a PC-relative call, large would get the current code sequence.</div><div><br></div><div>Juergen - I'll update the docs before committing.</div><div><br></div><div>Thanks for the review guys!</div><div><br></div><div>Cheers,</div><div>Lang.</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Apr 8, 2015 at 3:27 PM, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
  
    
  
  <div bgcolor="#FFFFFF" text="#000000">
    LGTM.  Since this is very similar to the statepoint code I used that
    for comparison sake.  <br>
    <br>
    In our local code we've got extra handling for
    ExternalSymbolSDNode.  I'm not actually sure if that's required
    though.  :)<br>
    <br>
    Here's the line we have:<br>
    else if (ExternalSymbolSDNode *ES = 
    dyn_cast<ExternalSymbolSDNode>(CallTarget))
    {                                                              <br>
     CallTarget =
    DAG.getTargetExternalSymbol(                                                                          
    <br>
        ES->getSymbol(), CallTarget.getValueType(),
    ES->getTargetFlags());                                             
    <br>
    }   <br>
    <br>
    One slight tweak you could make is to use a pc relative call rather
    than a register call for symbolic targets.  This avoids the need for
    an extra register, but does require that the target be within a
    fixed offset.  (Or maybe this can be handled via relocations?  Not
    sure.)<span class="HOEnZb"><font color="#888888"><br>
    <br>
    Philip</font></span><div><div class="h5"><br>
    <br>
    <div>On 03/31/2015 05:25 PM, Lang Hames
      wrote:<br>
    </div>
    <blockquote type="cite">
      <div dir="ltr">Hi All,
        <div><br>
        </div>
        <div>At the moment llvm.patchpoint call targets must be integer
          constants (e.g. 0xDEADBEEF). This patch adds support for
          symbolic targets like @foo, which addresses a couple of
          FIXMEs.</div>
        <div><br>
        </div>
        <div>Making this work just involved teaching FastISel and
          SelectionDAG to construct the appropriate MI/SDNodes, and
          teaching the Targets how to lower these to MCInsts. Target
          support for x86 is included in this patch. Support for other
          targets should be easy to derive from that.</div>
        <div><br>
        </div>
        <div>With this patch applied, you can write patchpoints of the
          following form:</div>
        <div><br>
        </div>
        <font face="monospace, monospace">tail call i64 (i64, i32, i8*,
          i32, ...)*</font>
        <div><span style="font-family:monospace,monospace"> 
            @llvm.experimental.patchpoint.void(i64 9, i32 15,</span></div>
        <div><span style="font-family:monospace,monospace">    i8*
            bitcast (i64 (i64, i64)* @foo to i8*),      ; <- Call
            target</span></div>
        <div><span style="font-family:monospace,monospace">    i32 2,
            i64 %p1, i64 %p2)</span>
          <div><br>
          </div>
          <div>and this will generate:</div>
          <div><br>
          </div>
          <font face="monospace, monospace">movabsq $_foo, %r11<br>
            callq   *%r11<br>
            // <nop-padding></font>
          <div><br>
          </div>
          <div>For integer targets this would have been something like:</div>
          <div><br>
          </div>
          <div><font face="monospace, monospace">movabsq $0xDEADBEEF,
              %r11<br>
              callq   *%r11<br>
              // <nop-padding></font><br>
          </div>
          <div><br>
          </div>
          <div>The advantage of symbolic targets, beyond improved
            readability, is that you can cache the IR or compiled
            objects and re-use them in contexts where the target address
            may have changed. For example, objects that use symbolic
            patchpoints can be cached in Orc/MCJIT object-caches and
            re-used across JIT invocations.</div>
          <div><br>
          </div>
          <div>Immediate-address targets are still fully supported.</div>
          <div><br>
          </div>
          <div>I don't really see any downside to this patch, but I
            thought I'd throw it out for general discussion before I go
            and widen what patchpoint is supposed to support.</div>
          <div><br>
          </div>
          <div>Does anyone see any problem with this idea?</div>
          <div><br>
          </div>
          <div>For the curious, my motivation is that I'd like
            (eventually) to support patchpoints for JIT re-entry in Orc
            as an alternative to indirect calls, and I want that
            transformation to be straightforward. E.g.</div>
          <div><br>
          </div>
          <div><font face="monospace, monospace">call @not_yet_compiled,
              ...</font></div>
          <div>to</div>
          <div><font face="monospace, monospace">call llvm.patchpoint
              ..., @not_yet_compiled, ...</font></div>
          <div><br>
          </div>
          <div>Cheers,</div>
          <div>Lang.</div>
        </div>
      </div>
    </blockquote>
    <br>
  </div></div></div>

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