[PATCH] D41381: [InstSimplify] Missed optimization in math expression: squashing exp(log), log(exp)

[Sanjay Patel via Phabricator via llvm-commits]

spatel accepted this revision.
spatel added a comment.

LGTM - see inline comments for some potential improvements.



================
Comment at: lib/Analysis/InstructionSimplify.cpp:4520
+      if (Q.CxtI->isFast()) {
+        Value *IIOperand = *ArgBegin;
+        Value *X = nullptr;
----------------
I don't see much value in naming this local (could just use *ArgBegin in the line below here).

So you could shrink this down to:
  Value *X;
  if (Q.CxtI->isFast() && match(*ArgBegin, m_Intrinsic<Intrinsic::log>(m_Value(X))))
    return X;
  return nullptr;


================
Comment at: lib/Analysis/InstructionSimplify.cpp:4521
+        Value *IIOperand = *ArgBegin;
+        Value *X = nullptr;
+        // exp(log(x)) -> x
----------------
As I've said in other reviews, I think explicitly setting this to nullptr is just noise.


================
Comment at: test/Transforms/InstSimplify/exp-intrinsic.ll:4-5
+
+declare double @llvm.exp.f64(double)
+declare double @llvm.log.f64(double)
+
----------------
It's a matter of taste, but since these are highly similar transforms, I'd put all of the tests together in one test file to match the fact that they're all handled together in one block of code.


================
Comment at: test/Transforms/InstSimplify/exp-intrinsic.ll:27-45
+define double @exp_fast_log_strict(double %a) {
+; CHECK-LABEL: @exp_fast_log_strict(
+; CHECK-NEXT:    ret double [[A:%.*]]
+;
+  %1 = call double @llvm.log.f64(double %a)
+  %2 = call fast double @llvm.exp.f64(double %1)
+  ret double %2
----------------
Looking back at the discussion in D5584, I think this is correct. Only the later op needs to be relaxed to do the transform.


https://reviews.llvm.org/D41381