[PATCH] [x86] make reciprocal estimate code generation more flexible

[Sanjay Patel]

================
Comment at: include/llvm/Target/TargetOptions.h:240
@@ -237,1 +239,3 @@
 
+    /// This class encapsulates options for reciprocal estimate code generation.
+    TargetRecip Reciprocals;
----------------
hfinkel wrote:
> reciprocal estimate -> reciprocal-estimate
Fixed.

================
Comment at: include/llvm/Target/TargetRecip.h:34
@@ +33,3 @@
+  VecSqrtD,     // square root, double, vector
+  INVALID
+};
----------------
spatel wrote:
> hfinkel wrote:
> > hfinkel wrote:
> > > To reduce confusion, I think it would be better to have a naming prefix on these. TailCallKind, for example, uses TCK_. Let's stick RO_ on these (including the INVALID one).
> > > 
> > If you're using INVALID for the "number of", please name it NUM_RECIP_OPS (or similar).
> > 
> With a named enum, I was expecting that users always have to use the prefix "RecipOps::" like I did in the x86 code.
> 
> But I see that's not done with TailCallKind...
Fixed.

================
Comment at: include/llvm/Target/TargetRecip.h:34
@@ +33,3 @@
+  VecSqrtD,     // square root, double, vector
+  INVALID
+};
----------------
spatel wrote:
> spatel wrote:
> > hfinkel wrote:
> > > hfinkel wrote:
> > > > To reduce confusion, I think it would be better to have a naming prefix on these. TailCallKind, for example, uses TCK_. Let's stick RO_ on these (including the INVALID one).
> > > > 
> > > If you're using INVALID for the "number of", please name it NUM_RECIP_OPS (or similar).
> > > 
> > With a named enum, I was expecting that users always have to use the prefix "RecipOps::" like I did in the x86 code.
> > 
> > But I see that's not done with TailCallKind...
> Fixed.
Fixed.

================
Comment at: include/llvm/Target/TargetRecip.h:48
@@ +47,3 @@
+  /// Set whether a particular reciprocal operation is enabled and how many
+  /// refinement steps are needed when using it. Use the INVALID operation
+  /// to set enablement and refinement steps for all operations.
----------------
hfinkel wrote:
> Let's not use INVALID for "all". You can add an All to the enum (it can even have the same value as INVALID).
Fixed - added an RO_All that is equal to RO_NUM_RECIP_OPS.

================
Comment at: lib/Target/TargetRecip.cpp:58
@@ +57,3 @@
+  // step parameter.
+  assert(RefStepString.length() == 1 && "Invalid refinement step for -recip.");
+  char RefStepChar = RefStepString[0];
----------------
hfinkel wrote:
> These strings are user-provided, we can't assert on invalid inputs.
Fixed - replaced with 'report_fatal_error'.

================
Comment at: lib/Target/TargetRecip.cpp:148
@@ +147,3 @@
+  unsigned NumStrings = llvm::array_lengthof(ArgStrings);
+  assert(NumArgs <= NumStrings && "Too many options for -recip.");
+
----------------
hfinkel wrote:
> Hrmm, there could be duplicates. Just parse them in order (users may provide duplicates).
Fixed. Still not sure if there's a decent way to test option parsing for llc, so there are very likely malformed input parsing bugs here.

http://reviews.llvm.org/D8982

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/