[PATCH] [x86] make reciprocal estimate code generation more flexible
Sanjay Patel
spatel at rotateright.com
Thu May 14 21:07:06 PDT 2015
================
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/
More information about the llvm-commits
mailing list