[PATCH] Make sure no loads resulting from load->switch DAGCombine are marked invariant

Thu Jul 31 13:21:02 PDT 2014

Well, if out of tree frontends use it enough to warrant keeping it for the easy case then lets fix this properly.  I hand audited all of the call sites and found a number of places where we we actually had info we could have passed and were dropping it on the floor.

New patch attached. It passes regression tests, but I still don’t really know how to write a decent test case for it.

Louis

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140731/412fa9e4/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Make-sure-no-loads-resulting-from-load-switch-DAGCom.patch
Type: application/octet-stream
Size: 39372 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140731/412fa9e4/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140731/412fa9e4/attachment-0001.html>