[PATCH] D19426: [AArch64] Use the reciprocal estimation machinery

[Evandro Menezes via llvm-commits]

evandro added a comment.

Thank you.


================
Comment at: llvm/lib/Target/AArch64/AArch64.td:67
@@ -61,1 +66,3 @@
+   "Enable the approximation of floating-point division", [FeatureNEON]>;
+
 //===----------------------------------------------------------------------===//
----------------
eastig wrote:
> evandro wrote:
> > eastig wrote:
> > > I think it's better to use names something like:
> > > FeatureReciprocalSqrtEstimate
> > > FeatureReciprocalEstimate
> > > 
> > > They don't directly calculate DIV and SQRT. They calculate aproximate 1/DIV and 1/SQRT.
> > Do you mean to replace HasApproximateSqrt with FeatureReciprocalSqrtEstimate and so on?
> Yes.
> When I first read the names of these definitions I had initial understanding based on their names. Later I saw instructions and from their documentation I found what they do. It didn't match to my initial understanding. I think when someone who is not familiar with these instructions starts looking at the code he or she will have the similar issue.
On the other hand, the reciprocals are later used to calculate the square root and division.  

All features start with "Has", that's why I did the same.  However, at the moment, the only features in use are ISA features, whereas this is an operation that, though it depends on an ISA feature, just makes use of it.  So, in a way, if it started with "Does" it might be more appropriate.  I'd appreciate to find out what others think.

But, if it makes too many waves dissipating in every direction, I'd rather fall back to the silly trains of "isCPUNAME" tests.

================
Comment at: llvm/lib/Target/AArch64/AArch64.td:142
@@ -135,1 +141,3 @@
+                                    FeaturePerfMon,
+                                    FeatureApproximateSqrt]>;
 
----------------
eastig wrote:
> evandro wrote:
> > eastig wrote:
> > > Why is FeatureApproximateDiv not on the list of the features?
> > Because Exynos M1 doesn't benefit from it.  It might be beneficial in other cores, when their respective maintainers should be better equipped than me to decide whether to add such features or not.
> Sorry for a naive question.
> Do we need to have these feature on the list? I thought they are part ARMv8-A NEON.
They are, but whether it's beneficial to always use them or not, beyond explicit intrinsics, is another matter that would depend on specific sub-targets.


Repository:
  rL LLVM

http://reviews.llvm.org/D19426