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

[Hal Finkel]

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

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