[PATCH] D65898: [InstCombine] x / fabs(x) -> copysign(1.0, x)

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1237
 
+  // X / fabs(X) -> copysign(1.0, X)
+  if (I.hasNoNaNs() && I.hasNoInfs() &&
----------------
Do we want to handle the commuted pattern too?
fabs(X) / X


================
Comment at: lib/Transforms/InstCombine/InstCombineMulDivRem.cpp:1240
+      match(Op1, m_Intrinsic<Intrinsic::fabs>(m_Specific(Op0))) &&
+      Op1->hasOneUse()) {
+    Value *V = Builder.CreateBinaryIntrinsic(
----------------
Do we need the one-use restriction?


================
Comment at: test/Transforms/InstCombine/fabs-copysign.ll:43
 
 define double @fabs_copysign_use(double %x) {
 ; CHECK-LABEL: @fabs_copysign_use(
----------------
This is supposed to be an extra use test, but there's no extra use?


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

https://reviews.llvm.org/D65898