[PATCH] D72643: [InstCombine] form copysign from select of FP constants (PR44153)

[Sanjay Patel via Phabricator via llvm-commits]

spatel marked 2 inline comments as done.
spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2331
+  if (!match(TVal, m_APFloat(TC)) || !match(FVal, m_APFloat(FC)) ||
+      !abs(*TC).bitwiseIsEqual(abs(*FC)) || TC == FC)
+    return nullptr;
----------------
nick wrote:
> I would never expect `TC == FC` to happen here. Are branches folded later than this transform?
We have to look at this 'select' independently of any other instruction because of the way instcombine processes. But I think you're correct that we can assume TC != FC because that should be simplified before we reach here. I will change that to an assert.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineSelect.cpp:2338
+  ICmpInst::Predicate Pred;
+  if (!match(Cond, m_OneUse(m_ICmp(Pred, m_BitCast(m_Value(X)), m_APInt(C)))) ||
+      !isSignBitCheck(Pred, *C, IsTrueIfSignSet) || X->getType() != SelType)
----------------
nick wrote:
> Is the one use check necessary? I would imagine that this transform will unblock other one use transforms.
It's not strictly necessary, but the transform becomes less clearly canonical in the case of >1 use because we can end up with more instructions than we started with (and I'm not sure what that will do in the backend/codegen). For those reasons, I think we should defer that change to its own minimal follow-on patch if we want to try it.


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

https://reviews.llvm.org/D72643