[PATCH] D94089: [Reassociate] move check to ignore boolean expressions before canonicalizing binary operands

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/Reassociate.cpp:2190-2195
+  // Do not reassociate boolean (i1) expressions.  We want to preserve the
+  // original order of evaluation for short-circuited comparisons that
+  // SimplifyCFG has folded to AND/OR expressions.  If the expression
+  // is not further optimized, it is likely to be transformed back to a
+  // short-circuited form for code gen, and the source order may have been
+  // optimized for the most likely conditions.
----------------
For reference, this was added in 2010:
27dfb1e

I'm sympathetic to the motivation, but that was a hack: one goal of IR is to overcome side-channel source-level optimization like this. 

For example, we added several IR transforms to reduce compiled-code differences in source-level diffs like "if (bool1 && bool2)" vs. "if (bool1 & bool2)" (logical vs. bitwise ops).

If the expression order matters, then we want the programmer to indicate that explicitly with something like "__builtin_expect" or profile metadata.

So this patch is really taking us away from the end goal (assuming we can get there). It might help solve the motivating problem in the short-term, but there's no guarantee that the fix will last.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D94089/new/

https://reviews.llvm.org/D94089