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

[Louis Gerbarg]

> 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.

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140731/b2aead06/attachment.html>