<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Tue, Oct 24, 2017 at 9:08 AM Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word;line-break:after-white-space"><div><br><blockquote type="cite"><div>On Oct 24, 2017, at 9:05 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:</div><br class="m_-5414322359623808608Apple-interchange-newline"><div><div dir="ltr">It merges as well as hoisting</div></div></blockquote><div><br></div></div></div><div style="word-wrap:break-word;line-break:after-white-space"><div>Ok. But re-reading the motivation of the patch, it looks like merging the locations (let's assume they are identical) would not actually solve the issue the hoisted constant's location being unexpected, right?</div><div>Were you suggesting to merge the location of the constant with the location of the InsertPoint?</div></div></blockquote><div><br>Yeah, I'm really not sure how to address it - if the constant is used in two different basic blocks but has the same location in both, but the constant is hoisted into a third basic block? We probably want to null out the location in that case (so I'm not sure merging it with the insertion point is ideal - letting the insertion point's location flow into this instruction if that's how it shakes out in the backend is fine, but actually specifying that location seems a bit off - if the instruction gets moved around some more, etc)<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word;line-break:after-white-space"><div><br></div><div></div></div><div style="word-wrap:break-word;line-break:after-white-space"><div>-- adrian</div></div><div style="word-wrap:break-word;line-break:after-white-space"><div><br><blockquote type="cite"><div><br><div class="gmail_quote"><div dir="ltr">On Tue, Oct 24, 2017 at 9:00 AM Adrian Prantl <<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
<br>
> On Oct 24, 2017, at 8:48 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
><br>
><br>
><br>
> On Tue, Sep 26, 2017 at 2:05 PM Robinson, Paul <<a href="mailto:paul.robinson@sony.com" target="_blank">paul.robinson@sony.com</a>> wrote:<br>
> Doesn't need to be merged with the insertion point, I don't think? Does this merge across basic blocks? or just raise the uses of a constant to the beginning of a single basic block?<br>
><br>
><br>
><br>
> I read the pass as looking for a dominating insertion point across multiple basic blocks.  Doesn't mean the insertion point necessarily is in a different block, but I *think* it could be.<br>
><br>
><br>
> Yeah, still seems like it might be nice to keep the location if it doesn't cross a basic block boundary.<br>
><br>
> Adrian - any ideas/thoughts on all this?<br>
<br>
Looks like I missed the original review... Does Constant Hoisting just move a single constant to the top, or does it merge multiple identical constants (while hoisting them)? In the latter case getMergedLocation should definitely be used. In the former case I'm not so sure if erasing the location is the right thing to do in the first place (but it probably doesn't hurt much in practice). So I guess my question is: What are you suggesting should the location be merged with?<br>
<br>
-- adrian<br>
</blockquote></div>
</div></blockquote></div></div></blockquote></div></div>