<html><head><style type='text/css'>p { margin: 0; }</style></head><body><div style='font-family: arial,helvetica,sans-serif; font-size: 10pt; color: #000000'><br><hr id="zwchr"><blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><b>From: </b>"Daniel Berlin" <dberlin@dberlin.org><br><b>To: </b>"Hal Finkel" <hfinkel@anl.gov><br><b>Cc: </b>reviews+D25542+public+6ffd7c85a26c1a77@reviews.llvm.org, "llvm-commits" <llvm-commits@lists.llvm.org>, "Sebastian Pop" <sebpop@gmail.com><br><b>Sent: </b>Monday, November 7, 2016 8:08:37 PM<br><b>Subject: </b>Re: [PATCH] D25542: GVN-hoist: avoid calling MemoryLocation::get() on a call (PR30499)<br><br><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Nov 7, 2016 at 5:37 PM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">Hi Danny,<br>
<br>
MemoryLocation does not apply to calls directly because a call can easily affect multiple locations. </blockquote><div><br></div><div><br></div><div>Yes, but people often just want to know "does instruction def/ref/nothing the memory of other instruction"</div><div><br></div><div>They don't often want to care about exactly what type of instruction they are holding in their hand, and at least right now, it doesn't just work all the time, you have to know what you are holding in your hands.</div><div><br></div><div>They also don't often care about specific locations.</div><div><br></div><div id="DWT36982">If we think we can make that work all the time, awesome, let's do that.</div></div></div></div></blockquote>This is exactly my point. I see no reason not to make this "just work." Normally, this is exactly what you want: I have Instruction *I1 and Instruction *I2, tell me about the aggregated relative aliasing of any memory they access. The fact that we can't do this I don't think came from any particular design, just from the way the interface has evolved.<br><br>FWIW, We also currently have an issue that our mod/ref interface does not give us MayAlias vs MustAlias differentiation (etc.). So there are some use cases that require using the underlying MemoryLocation-based interface. There are other potential uses for MemoryLocation, such as when you want to take the address and change the size (or the offset, but unfortunately, we can't do this right now).<br><br><blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div id="DWT35850">Otherwise, literally everyone i know who has ever written a memory optimization has run into this issue :P</div></div></div></div></blockquote>Yes, and having written a significant amount of this code myself, I too have found the state of things unfortunate.<br><br> -Hal<br><blockquote style="border-left: 2px solid rgb(16, 16, 255); margin-left: 5px; padding-left: 5px; color: rgb(0, 0, 0); font-weight: normal; font-style: normal; text-decoration: none; font-family: Helvetica,Arial,sans-serif; font-size: 12pt;"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div><br></div><div>(The further problem here being the one you expect, we expect people to manually phi translate things like pointers, which means if they want the best answers, they have to try to use MemoryLocation when they can, and phi translate it appropriately)</div><div><br></div><div><br></div><div>Don't get me wrong, there are times when people want *more* detailed answers (ie the second getmodrefinfo you have below) , but from what i've seen, that's pretty rare.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">We could add a MemoryLocations (plural), but we'd also want to capture the read/write sense of each location, and then we still run the risk of losing the ability to capture specific call-vs-call aliasing logic.<br>
<br>
We have:<br>
<br>
ModRefInfo AAResults::getModRefInfo(ImmutableCallSite CS1,<br>
ImmutableCallSite CS2);<br>
<br>
and<br>
<br>
ModRefInfo AAResults::getModRefInfo(ImmutableCallSite CS,<br>
const MemoryLocation &Loc);<br>
<br>
and I wonder whether it would be better to encourage developers to always use the mod/ref interfaces when we might be dealing with calls (as single entities). I suspect that we should probably be using mod/ref more than we do now anyway.<br></blockquote><div><br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;"><br>
I think that we should fix the mod/ref functions so that all of the various combinations of underlying instruction types just work.<br>
<br>
-Hal<br>
<span class=""><br>
<hr id="zwchr"><br>
> From: "Daniel Berlin" <<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>><br>
> To: "Sebastian Pop" <<a href="mailto:sebpop@gmail.com" target="_blank">sebpop@gmail.com</a>><br>
> Cc: <a href="mailto:reviews%2BD25542%2Bpublic%2B6ffd7c85a26c1a77@reviews.llvm.org" target="_blank">reviews+D25542+public+6ffd7c85a26c1a77@reviews.llvm.org</a>, "llvm-commits" <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>>, "Hal<br>
> Finkel" <<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>><br>
> Sent: Wednesday, October 12, 2016 10:40:48 PM<br>
> Subject: Re: [PATCH] D25542: GVN-hoist: avoid calling MemoryLocation::get() on a call (PR30499)<br>
><br>
><br>
> (as an aside, Hal, i'd love your opinion on whether MemoryLocation<br>
> should really be something that handles calls as well. Or,<br>
> alternatively, at least getModRefInfo uses something that does<br>
> without people having to know about it.<br>
><br>
><br>
> If we used something like MemoryLocOrCall [obviously this is just a<br>
> simple union class]), we could simplify parts of the AA API. I also<br>
> wouldn't need it to hack around things in the AA API's<br>
> getModRefInfo.<br>
><br>
><br>
><br>
> Right now we have this artificial distinction where we have things<br>
> that use pointers get MemoryLocations, other things get<br>
> ImmutableCallSites. This kinda makes sense for alias() (though more<br>
> advanced aa's are just implementing that in terms of mod ref info<br>
> they have created).<br>
><br>
><br>
> However, the current AA API operates on both *anyway*, and the<br>
> overloads you get are confusing.<br>
><br>
><br>
> IE we have:<br>
</span>> 00333 ModRefInfo getModRefInfo ( ImmutableCallSite CS , const<br>
> MemoryLocation &Loc);<br>
> and<br>
> 00751 ModRefInfo getModRefInfo ( ImmutableCallSite CS1,<br>
<div class="HOEnZb"><div class="h5">> ImmutableCallSite CS2);<br>
><br>
><br>
> etc<br>
><br>
><br>
><br>
><br>
> You pretty much can't use getModRefInfo right now unless you check<br>
> whether the instructions you are trying to get mod ref info about<br>
> are 0, 1, or 2 calls. Because calls don't have memorylocs, so you<br>
> can't just always call ::get on them.<br>
> You then have to call the right overloads yourself.<br>
> That seems wrong.<br>
> (I'll ignore the fact that you also have to check whether you are<br>
> holding a fence inst, since you can't get memorylocations for those<br>
> either, and ::get will just crash. )<br>
><br>
><br>
> Right now the number of overloads is also ... high, and while we<br>
> fixed the last immediate problem i remember (where random things<br>
> would get overloaded to ImmutableCallSite versions when they weren't<br>
> calls), staring at the number of getModRefInfo overloads, i'm not<br>
> sure i believe the way we have it structured makes sense.<br>
><br>
><br>
><br>
><br>
><br>
><br>
> On Wed, Oct 12, 2016 at 8:26 PM, Daniel Berlin < <a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a><br>
> > wrote:<br>
><br>
><br>
><br>
> You can get rid of UseInst too, MemoryLocOrCall will take a MemoryUse<br>
> directly :)<br>
><br>
><br>
><br>
><br>
><br>
><br>
> On Wed, Oct 12, 2016 at 8:20 PM, Sebastian Pop < <a href="mailto:sebpop@gmail.com" target="_blank">sebpop@gmail.com</a> ><br>
> wrote:<br>
><br>
><br>
> On Wed, Oct 12, 2016 at 10:05 PM, Daniel Berlin < <a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a><br>
> > wrote:<br>
> > There is actually a much simpler fix you sholdl use.<br>
> ><br>
> > return instructionClobbersQuery(MD, MU MemoryLocOrCall(MU), AA);<br>
> ><br>
> > You need to move that version of instructionClobbersQuery up, but<br>
> > it was<br>
> > built to always do the right thing.<br>
> ><br>
> > Also note it will properly handle a case you are not: Fences do not<br>
> > have<br>
> > memorylocations, and calling get on them will also crash.<br>
> ><br>
><br>
> Right. I will commit the attached patch after testing.<br>
><br>
> Thanks,<br>
> Sebastian<br>
><br>
><br>
><br>
<br>
</div></div><span class="HOEnZb"><font color="#888888">--<br>
Hal Finkel<br>
Lead, Compiler Technology and Programming Languages<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</font></span></blockquote></div><br></div></div>
</blockquote><br><br><br>-- <br><div><span name="x"></span>Hal Finkel<br>Lead, Compiler Technology and Programming Languages<br>Leadership Computing Facility<br>Argonne National Laboratory<span name="x"></span><br></div></div></body></html>