[PATCH] make reciprocal estimate code generation more flexible by adding command-line options

hfinkel at anl.gov hfinkel at anl.gov
Fri May 22 13:12:48 PDT 2015


LGTM.


================
Comment at: lib/Target/X86/X86TargetMachine.cpp:108
@@ -107,2 +107,3 @@
 
+  this->Options.Reciprocals.setDefaults("all", false, 1);
   initAsmInfo();
----------------
spatel wrote:
> hfinkel wrote:
> > spatel wrote:
> > > hfinkel wrote:
> > > > spatel wrote:
> > > > > hfinkel wrote:
> > > > > > Why false? Do you want a target feature here?
> > > > > We had target features to control these, but I think it would be better to behave like gcc unless we have reason to diverge. 
> > > > > 
> > > > > That said, this does not match gcc behavior yet; that would be my next patch. For x86 at least, we would turn the following on by default when using -ffast-math:
> > > > > sqrt
> > > > > vec-sqrt
> > > > > vec-div
> > > > > 
> > > > > I didn't set these defaults in this patch because it would change -ffast-math codegen for all CPUs other than btver2 (which had the recip codegen enabled for all eligible x86 recip types via target features).
> > > > I thought that getRsqrtEstimate, etc. were only called when fast-math is on?
> > > That's correct; everything is gated by -ffast-math or an unsafe algebra equivalent. Recip-est codegen should only be active after that check.
> > > 
> > > With gcc, once you have -ffast-math, you also get -mrecip=sqrt,vec-sqrt,vec-div by default. AFAIK, that is independent of arch or CPU subtarget.
> > > 
> > > The lone exception is 'div' (scalar division). Estimating scalar div on x86 (no FMA until recently) breaks a lot of real world code, so I want to keep that off by default (and again match gcc default behavior).
> > But you turn them all off here, right? So that does not match gcc. Please add a comment here explaining what is going on.
> Right - my intent was to separate the structural change from the functional change as much as possible. 
> 
> We don't currently generate any reciprocal estimates for x86 with -ffast-math except when targeting btver2. So all x86 codegen should be identical after this patch except in the case where someone has specified -mcpu=btver2 -ffast-math.
> 
> I'll send the patch to change this default behavior to match GCC defaults as soon as possible, but I wanted to keep that separate in case anyone does not agree that we should change that default behavior. PPC and ARM recip patches would also be independent but similar (assuming they want to match GCC too).
> 
Okay, but please add a comment explaining the current state of things, with a TODO, here (maybe the follow-up with happen soon, but in case there's trouble, the code will be clear in the mean time).

http://reviews.llvm.org/D8982

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






More information about the llvm-commits mailing list