<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div><div><div><br><div><blockquote type="cite"><div>On Jul 31, 2014, at 11:03 AM, Hal Finkel <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>> wrote:</div><br class="Apple-interchange-newline"><div><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;">----- Original Message -----</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">From: "Louis Gerbarg" <<a href="mailto:lgg@apple.com">lgg@apple.com</a>><br>To: "Hal Finkel" <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>><br>Cc: <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>Sent: Thursday, July 31, 2014 12:47:39 PM<br>Subject: Re: [PATCH] Make sure no loads resulting from load->switch DAGCombine are<span class="Apple-tab-span" style="white-space: pre;"> </span>marked invariant<br><br><br><br><br><br><br>On Jul 31, 2014, at 8:23 AM, Hal Finkel < <a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a> > wrote:<br><br>----- Original Message -----<br><br><br>From: "Louis Gerbarg" < <a href="mailto:lgg@apple.com">lgg@apple.com</a> ><br>To: <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>Sent: Wednesday, July 30, 2014 4:52:17 PM<br>Subject: [PATCH] Make sure no loads resulting from load->switch<br>DAGCombine are marked invariant<br><br><br><br>While discussing r214322 <<br><a href="http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140728/228518.html">http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140728/228518.html</a><br><br><br>it was mentioned that similar fixes should be considered for<br>isInvariant and isNonTemporal. I looked at those along with<br>isNonvolatile. Here is what I found:<br><br>isNontemporal:<br>The current behavior will not result in miscompiles and is<br>reasonable. Incorrectly marking or unmarking a nontemporal variable<br>is just a slight performance hit, the results will be the same. In<br>the case where both inputs are the same taking the left side is<br>optimal, in the case where they differ we need to make a choice and<br>barring any other data taking the left side value makes sense just<br>because it requires no additional code.<br><br>isVolatile:<br>At first glance the current behavior appears to be wrong as it is<br>just taking the volatility of the left side into account. Tracking a<br>bit back through the code path it turns out that if either side is<br>volatile the combine bails out early, so the left and right are<br>always non-volatile. It is probably worth getting rid of that check<br>and making the resultant combine create a volatile load, but as that<br>is not a correctness issue it is not too pressing. It also has some<br>testing concerns (the same ones as isInvariant, see below).<br><br>isInvariant:<br>This case is just wrong. If a variable load is accidentally marked<br>invariant it can result in bad codegen, and if we combine two loads<br>where the left is invariant the right is not that is exactly what<br>will happen. While I could write code to test if they are both<br>invariant and propagate that it doesn’t seem worthwhile as it is a<br>rarely used bit of metadata and stripping it is always safe.<br>Additionally, if we propagate it I have no idea how to test it…<br>since this a DAGCombine I can only lit the resultant assembly, but<br>this should not generate different instructions, just introduce more<br>flexibility in the instruction scheduler. While I can certainly<br>build some large basic blocks that demonstrate a difference in<br>behavior I think they would all be fairly fragile with respect to<br>codegen and scheduling changes.<br><br>Attached is a patch to disable drops the isInvariant flag during the<br>combine. No tests included, let me know if you have any ideas about<br>how to test this.<br><br>Why don't, for the regular load case, we use (LLD->isInvariant() &&<br>RLD->isInvariant()) instead of just hard-coding 'false'? I'd prefer<br>that we not through away the information when we legitimately have<br>it.<br><br><br>I understand your concern and in general I am not a fan of throwing<br>out any data we have. In this case there are two interrelated<br>reasons why I just set it to false:<br><br><br>1) getExtLoad() doesn’t actually allow you pass it in. So in order to<br>do it I would need to make another overload of getExtLoad() just for<br>this call site or update 90 call sites throughout the whole project.<br>I could do it for the getLoad() case, but then the two sides of the<br>if statement would behave subtly differently in a way that is<br>non-obvious upon cursory examination.<br>2) This metadata is, as I understand it, exclusively used by Clang<br>code when it is effectively lazily loading ivar offsets or compiled<br>in selector addresses of ObjC code. Those are case where we are<br>effectively splatting down a very specific structure that won’t end<br>up going through this combine anyway.<br><br>I’m relatively loathe to add the (admittedly minor) maintenance<br>burden of another overload to getExtLoad() or the massive patch to<br>change all of its call sites given that as far as I am aware the<br>metadata will never be used in cases where this combine is happening<br>anyway. There is already a FIXME there about the combine dropping<br>some other info right there, I would be more than happy to add a<br>comment about Invariant to it.<br><br>If some other frontend makes use of this metadata and it is in fact<br>valuable to keep it through the combine then my preference would be<br>to update the existing getExtLoad() and update the call sites.<br></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;">While I can't comment on the ObjC use case, I work with several developers who have custom frontends, and I believe that marking loads as invariant is used (mostly so that they can be hoisted out of loops, latency hiding, etc.). I think that we should at least preserve this for normal loads, and if you'd update getExtLoad as well, that'd be great!</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;">Thanks again,</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;">Hal</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><blockquote type="cite" style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><br><br>Louis<br><br><br><br>Thanks again,<br>Hal<br><br><br><br><br>Louis<br><br><br><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits<br><br><br>--<br>Hal Finkel<br>Assistant Computational Scientist<br>Leadership Computing Facility<br>Argonne National Laboratory<br><br></blockquote><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;">--<span class="Apple-converted-space"> </span></span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;">Hal Finkel</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;">Assistant Computational Scientist</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;">Leadership Computing Facility</span><br style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;"><span style="font-family: Helvetica; font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px; float: none; display: inline !important;">Argonne National Laboratory</span></div></blockquote></div><br></div></div></div></body></html>