[PATCH] D60037: [PowerPC] Use the two-constant NR algorithm for refining estimates

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added a comment.

I have a couple of nits. The main one being the test case for `fma-mutate.ll`.
I'm not sure if there is a way to use one const NR for just that one test.



================
Comment at: lib/Target/PowerPC/PPC.td:140
+  SubtargetFeature<"two-const-nr", "NeedsTwoConstNR", "true",
+                   "Requires two constant Newton-Rhapson computation">;
 def FeatureP8Altivec : SubtargetFeature<"power8-altivec", "HasP8Altivec", "true",
----------------
nit:
I think it is "Newton-Raphson" not "Newton-Rhapson".


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:10988
 
-    UseOneConstNR = true;
+    // The Newton-Rhapson computation with a single constant does not provide
+    // enough accuracy on some CPUs.
----------------
Same, nit:
Newton-Raphson


================
Comment at: test/CodeGen/PowerPC/fma-mutate.ll:17
 ; CHECK: @foo3
-; CHECK: fmr [[REG:[0-9]+]], [[REG2:[0-9]+]]
-; CHECK: xsnmsubadp [[REG]], {{[0-9]+}}, [[REG2]]
 ; CHECK: xsmaddmdp
 ; CHECK: xsmaddadp
----------------
With this new patch this test no longer really tests all of the things that the author wanted to test.

As the author of the test mentions, the first transformation is not reasonable. However, the potential opportunity for this first transformation is no longer there (the `fmr` instruction for it is gone) with two const Newton-Raphson. We can at least detect the second legal transformation with the CHECK-NOT.
```
; CHECK: @foo3
; CHECK-NOT: fmr
; CHECK: xsmaddmdp
; CHECK: xsmaddadp
```



Repository:
  rL LLVM

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

https://reviews.llvm.org/D60037