<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Jun 27, 2017 at 12:01 PM, Eli Friedman via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">efriedma added inline comments.<br>
<br>
<br>
================<br>
Comment at: lib/Transforms/Utils/<wbr>CodeExtractor.cpp:79<br>
<span class="">+    if (isa<BlockAddress const>(Curr))<br>
</span>+      return false; // even a reference to self is likely to be not compatible<br>
+<br>
----------------<br>
I'm not sure I follow this check.<br>
<br>
You can't extract a basic block whose terminator is an indirectbr; in general, you can't efficiently break the relevant CFG edges.  Similarly, you can't extract a block with an indirectbr predecessor.  And extracting a block whose address is taken is likely to lead to unexpected results in other cases for code which abuses block addresses for other uses (like the Linux kernel).<br></blockquote><div><br></div><div><br></div><div>If  all the indirectbr predecessors and the target block are in the extracted region, it should be fine to extract the block.  Of course this patch is more conservative.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
But I can't see why you would want to block extracting a basic block just because it refers to a blockaddress, or a global variable which contains a blockaddress.<br></blockquote><div><br></div><div><br></div><div>What you are saying is that the code should directly check the use of indirectbr instruction instead which sounds better.</div><div><br></div><div>David</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="HOEnZb"><div class="h5"><br>
<br>
Repository:<br>
  rL LLVM<br>
<br>
<a href="https://reviews.llvm.org/D33839" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D33839</a><br>
<br>
<br>
<br>
</div></div></blockquote></div><br></div></div>