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

Louis Gerbarg lgg at apple.com
Wed Jul 30 14:52:17 PDT 2014


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.

Louis

-------------- 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: 1663 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140730/a6e14383/attachment.obj>


More information about the llvm-commits mailing list