[PATCH] D40304: [InstCombine] PR35354: Convert load bitcast (select (Cond, &V1, &V2)) --> select(Cond, load bitcast &V1, load bitcast &V2)

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:566
+///   select ((cmp load V1, load V2), V1, V2).
+static bool isMinMaxPattern(Value *V) {
+  assert(V->getType()->isPointerTy() && "Expected pointer type.");
----------------
spatel wrote:
> Why do we need to match min/max? Does something break if we just match select?
> 
> If we need to restrict to min/max, this isn't the way to do it. Use pattern matches (m_c_SMax, etc)  or value tracking's matchSelectPattern().
1. Currently, we have troubles only with particular minmax pattern, not all select instructions. I think, at first we should do this for minmax. If it is required for other select-based patterns, we can extend this patch or remove it.
2. Ok, will do it via pattern matchers. Though I can use m_c_<Matcher> directly, because we have a slightly different minmax pattern (comparing of values, but return addresses of this values, not values themselves).


================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:1333
+/// select ((cmp load V1, load V2), V1, V2).
+bool decanonicalizeLoadStoreOnSelect(InstCombiner &IC, StoreInst &SI) {
+  // bitcast?
----------------
spatel wrote:
> It doesn't make sense to call this  "decanonicalize". Whatever we choose to do here is redefining canonical. "removeBitcastsFromLoadStoreOnSelect"?
Ok, will rename it.


https://reviews.llvm.org/D40304