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

Hal Finkel hfinkel at anl.gov
Thu Jul 31 11:03:08 PDT 2014


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




More information about the llvm-commits mailing list