<div dir="ltr">One more patch attached.</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Jun 12, 2013 at 5:49 PM, Akira Hatanaka <span dir="ltr"><<a href="mailto:ahatanak@gmail.com" target="_blank">ahatanak@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">When I run my test program with a negative input, the latter approach is about 10% faster.<div>The former approach is faster when the input is positive, but the difference is smaller (3-4%).<br>
<div class="gmail_extra">

<br>I created a patch which moves this optimization to lib/CodeGen. You can try optimizing sqrt for other targets with these commands:</div><div class="gmail_extra"><br></div><div class="gmail_extra">$  llc test-sqrt2.ll -o - -enable-math-optimization (this is for checking nan)</div>


<div class="gmail_extra"><div class="gmail_extra">$  llc test-sqrt2.ll -o - -enable-math-optimization -check-neg-fp (this is for checking negative)</div><div><br></div><div>Note that in order to emit a native sqrt instruction, the target needs a fsqrt pattern.</div>


</div><div class="gmail_extra">I tried this pass on x86, but didn't see a large difference between the two approaches.</div><div><div class="h5"><div class="gmail_extra"><br></div><div class="gmail_extra"><div class="gmail_quote">
On Tue, Jun 11, 2013 at 10:27 PM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin-top:0px;margin-right:0px;margin-bottom:0px;margin-left:0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">----- Original Message -----<br>



><br>
<div>> I ran a simple experiment to compare the two approaches. I don't have<br>
> the exact numbers yet, but the approach in this patch is slightly<br>
> faster than the approach that checks whether the input is negative,<br>
> when the input is always non-negative. The difference is not<br>
> significant, but is still measurable. One thing I noticed studying<br>
> the generated code is that mips needs extra instructions (either a<br>
> load or copy from register $0) and an extra register to load<br>
> constant 0.0, which might explain the difference.<br>
><br>
><br>
> When the input is negative, the latter approach was faster as<br>
> expected.<br>
<br>
</div>Just a little faster, or ~2x faster? I think that this makes a big difference. As a user, I've been bitten by this kind of transformation before, and find it quite annoying when applications exhibit huge slowdowns when NaNs start being generated.<br>



<br>
Thanks again,<br>
Hal<br>
<div><div><br>
><br>
> I'll continue looking into this.<br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
> On Tue, Jun 11, 2013 at 4:13 PM, Hal Finkel < <a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a> ><br>
> wrote:<br>
><br>
><br>
><br>
> ----- Original Message -----<br>
> ><br>
> ><br>
> > I was simply copying what gcc was doing. As you've pointed out, you<br>
> > can check if the input is negative first. The difference is that<br>
> > you'll have to emit one more branch (not sure how much difference<br>
> > this will make to performance):<br>
><br>
> I think this is worth testing. I'd guess that, assuming you arrange<br>
> the branches so that it defaults to calling the native instruction,<br>
> the cost of the extra branch will be small, and you won't have the<br>
> >2x runtime increase on negative inputs.<br>
><br>
> -Hal<br>
><br>
><br>
><br>
> ><br>
> ><br>
> > if (input is negative)<br>
> ><br>
> > libcall<br>
> > else<br>
> > native instruction<br>
> ><br>
> ><br>
> ><br>
> ><br>
> > (transformation in this patch):<br>
> ><br>
> ><br>
> > native_instruction<br>
> ><br>
> ><br>
> ><br>
> ><br>
> > if (result is nan)<br>
> > libcall<br>
> ><br>
> ><br>
> > I made this pass mips-specific because I was planning to commit<br>
> > another patch which does a mips-specific optimization to calls to<br>
> > other math library calls (ceil, trunc and trunc), but there isn't<br>
> > any reason the transformation in this patch can't be target<br>
> > independent.<br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
> > On Tue, Jun 11, 2013 at 3:45 PM, Hal Finkel < <a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a> ><br>
> > wrote:<br>
> ><br>
> ><br>
> ><br>
> > ----- Original Message -----<br>
> > > Author: ahatanak<br>
> > > Date: Tue Jun 11 17:21:44 2013<br>
> > > New Revision: 183802<br>
> > ><br>
> > > URL: <a href="http://llvm.org/viewvc/llvm-project?rev=183802&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=183802&view=rev</a><br>
> > > Log:<br>
> > > [mips] Add an IR transformation pass that optimizes calls to<br>
> > > sqrt.<br>
> > ><br>
> > > The pass emits a call to sqrt that has attribute "read-none".<br>
> > > This<br>
> > > call will be<br>
> > > converted to an ISD::FSQRT node during DAG construction, which<br>
> > > will<br>
> > > turn into<br>
> > > a mips native sqrt instruction.<br>
> ><br>
> > This seems almost completely target independent, is there a reason<br>
> > to<br>
> > make this MIPS-specific? Also, is it really better to test the<br>
> > result for NaN as opposed to checking whether the input is<br>
> > negative?<br>
> ><br>
> > -Hal<br>
> ><br>
> ><br>
> ><br>
> > ><br>
> > ><br>
> > > Added:<br>
> > > llvm/trunk/lib/Target/Mips/MipsOptimizeMathLibCalls.cpp<br>
> > > llvm/trunk/test/CodeGen/Mips/optimize-fp-math.ll<br>
> > > Modified:<br>
> > > llvm/trunk/lib/Target/Mips/Mips.h<br>
> > > llvm/trunk/lib/Target/Mips/MipsTargetMachine.cpp<br>
> > ><br>
> > > Modified: llvm/trunk/lib/Target/Mips/Mips.h<br>
> > > URL:<br>
> > > <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/Mips/Mips.h?rev=183802&r1=183801&r2=183802&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/Mips/Mips.h?rev=183802&r1=183801&r2=183802&view=diff</a><br>



> > > ==============================================================================<br>
> > > --- llvm/trunk/lib/Target/Mips/Mips.h (original)<br>
> > > +++ llvm/trunk/lib/Target/Mips/Mips.h Tue Jun 11 17:21:44 2013<br>
> > > @@ -28,7 +28,7 @@ namespace llvm {<br>
> > > FunctionPass *createMipsJITCodeEmitterPass(MipsTargetMachine &TM,<br>
> > > JITCodeEmitter &JCE);<br>
> > > FunctionPass *createMipsConstantIslandPass(MipsTargetMachine<br>
> > > &tm);<br>
> > > -<br>
> > > + FunctionPass *createMipsOptimizeMathLibCalls(MipsTargetMachine<br>
> > > &TM);<br>
> > > } // end namespace llvm;<br>
> > ><br>
> > > #endif<br>
> > ><br>
> > > Added: llvm/trunk/lib/Target/Mips/MipsOptimizeMathLibCalls.cpp<br>
> > > URL:<br>
> > > <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/Mips/MipsOptimizeMathLibCalls.cpp?rev=183802&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/Mips/MipsOptimizeMathLibCalls.cpp?rev=183802&view=auto</a><br>



> > > ==============================================================================<br>
> > > --- llvm/trunk/lib/Target/Mips/MipsOptimizeMathLibCalls.cpp<br>
> > > (added)<br>
> > > +++ llvm/trunk/lib/Target/Mips/MipsOptimizeMathLibCalls.cpp Tue<br>
> > > Jun<br>
> > > 11 17:21:44 2013<br>
> > > @@ -0,0 +1,175 @@<br>
> > > +//===---- MipsOptimizeMathLibCalls.cpp - Optimize math lib<br>
> > > calls.<br>
> > > ----===//<br>
> > > +//<br>
> > > +// The LLVM Compiler Infrastructure<br>
> > > +//<br>
> > > +// This file is distributed under the University of Illinois<br>
> > > Open<br>
> > > Source<br>
> > > +// License. See LICENSE.TXT for details.<br>
> > > +//<br>
> > > +//===----------------------------------------------------------------------===//<br>
> > > +//<br>
> > > +// This pass does an IR transformation which enables the backend<br>
> > > to<br>
> > > emit native<br>
> > > +// math instructions.<br>
> > > +//<br>
> > > +//===----------------------------------------------------------------------===//<br>
> > > +<br>
> > > +#include "MipsTargetMachine.h"<br>
> > > +#include "llvm/IR/IRBuilder.h"<br>
> > > +#include "llvm/IR/Intrinsics.h"<br>
> > > +#include "llvm/Pass.h"<br>
> > > +#include "llvm/Support/CommandLine.h"<br>
> > > +#include "llvm/Target/TargetLibraryInfo.h"<br>
> > > +#include "llvm/Transforms/Utils/BasicBlockUtils.h"<br>
> > > +<br>
> > > +using namespace llvm;<br>
> > > +<br>
> > > +static cl::opt<bool><br>
> > > DisableOpt("disable-mips-math-optimization",<br>
> > > + cl::init(false),<br>
> > > + cl::desc("MIPS: Disable math lib<br>
> > > call "<br>
> > > + "optimization."),<br>
> > > cl::Hidden);<br>
> > > +<br>
> > > +namespace {<br>
> > > + class MipsOptimizeMathLibCalls : public FunctionPass {<br>
> > > + public:<br>
> > > + static char ID;<br>
> > > +<br>
> > > + MipsOptimizeMathLibCalls(MipsTargetMachine &TM_) :<br>
> > > + FunctionPass(ID), TM(TM_) {}<br>
> > > +<br>
> > > + virtual const char *getPassName() const {<br>
> > > + return "MIPS: Optimize calls to math library functions.";<br>
> > > + }<br>
> > > +<br>
> > > + virtual void getAnalysisUsage(AnalysisUsage &AU) const;<br>
> > > +<br>
> > > + virtual bool runOnFunction(Function &F);<br>
> > > +<br>
> > > + private:<br>
> > > + /// Optimize calls to sqrt.<br>
> > > + bool optimizeSQRT(CallInst *Call, Function *CalledFunc,<br>
> > > + BasicBlock &CurrBB,<br>
> > > + Function::iterator &BB);<br>
> > > +<br>
> > > + const TargetMachine &TM;<br>
> > > + };<br>
> > > +<br>
> > > + char MipsOptimizeMathLibCalls::ID = 0;<br>
> > > +}<br>
> > > +<br>
> > > +FunctionPass<br>
> > > *llvm::createMipsOptimizeMathLibCalls(MipsTargetMachine<br>
> > > &TM) {<br>
> > > + return new MipsOptimizeMathLibCalls(TM);<br>
> > > +}<br>
> > > +<br>
> > > +void MipsOptimizeMathLibCalls::getAnalysisUsage(AnalysisUsage<br>
> > > &AU)<br>
> > > const {<br>
> > > + AU.addRequired<TargetLibraryInfo>();<br>
> > > + FunctionPass::getAnalysisUsage(AU);<br>
> > > +}<br>
> > > +<br>
> > > +bool MipsOptimizeMathLibCalls::runOnFunction(Function &F) {<br>
> > > + if (DisableOpt)<br>
> > > + return false;<br>
> > > +<br>
> > > + const MipsSubtarget &Subtarget =<br>
> > > TM.getSubtarget<MipsSubtarget>();<br>
> > > +<br>
> > > + if (Subtarget.inMips16Mode())<br>
> > > + return false;<br>
> > > +<br>
> > > + bool Changed = false;<br>
> > > + Function::iterator CurrBB;<br>
> > > + const TargetLibraryInfo *LibInfo =<br>
> > > &getAnalysis<TargetLibraryInfo>();<br>
> > > +<br>
> > > + for (Function::iterator BB = F.begin(), BE = F.end(); BB !=<br>
> > > BE;)<br>
> > > {<br>
> > > + CurrBB = BB++;<br>
> > > +<br>
> > > + for (BasicBlock::iterator II = CurrBB->begin(), IE =<br>
> > > CurrBB->end();<br>
> > > + II != IE; ++II) {<br>
> > > + CallInst *Call = dyn_cast<CallInst>(&*II);<br>
> > > + Function *CalledFunc;<br>
> > > +<br>
> > > + if (!Call || !(CalledFunc = Call->getCalledFunction()))<br>
> > > + continue;<br>
> > > +<br>
> > > + LibFunc::Func LibFunc;<br>
> > > + Attribute A = CalledFunc->getAttributes()<br>
> > > + .getAttribute(AttributeSet::FunctionIndex,<br>
> > > "use-soft-float");<br>
> > > +<br>
> > > + // Skip if function has "use-soft-float" attribute.<br>
> > > + if ((A.isStringAttribute() && (A.getValueAsString() ==<br>
> > > "true")) ||<br>
> > > + TM.Options.UseSoftFloat)<br>
> > > + continue;<br>
> > > +<br>
> > > + // Skip if function either has local linkage or is not a known<br>
> > > library<br>
> > > + // function.<br>
> > > + if (CalledFunc->hasLocalLinkage() || !CalledFunc->hasName() ||<br>
> > > + !LibInfo->getLibFunc(CalledFunc->getName(), LibFunc))<br>
> > > + continue;<br>
> > > +<br>
> > > + switch (LibFunc) {<br>
> > > + case LibFunc::sqrtf:<br>
> > > + case LibFunc::sqrt:<br>
> > > + if (optimizeSQRT(Call, CalledFunc, *CurrBB, BB))<br>
> > > + break;<br>
> > > + continue;<br>
> > > + default:<br>
> > > + continue;<br>
> > > + }<br>
> > > +<br>
> > > + Changed = true;<br>
> > > + break;<br>
> > > + }<br>
> > > + }<br>
> > > +<br>
> > > + return Changed;<br>
> > > +}<br>
> > > +<br>
> > > +bool MipsOptimizeMathLibCalls::optimizeSQRT(CallInst *Call,<br>
> > > + Function *CalledFunc,<br>
> > > + BasicBlock &CurrBB,<br>
> > > + Function::iterator &BB)<br>
> > > {<br>
> > > + // There is no need to change the IR, since backend will emit<br>
> > > sqrt<br>
> > > + // instruction if the call has already been marked read-only.<br>
> > > + if (Call->onlyReadsMemory())<br>
> > > + return false;<br>
> > > +<br>
> > > + // Do the following transformation:<br>
> > > + //<br>
> > > + // (before)<br>
> > > + // dst = sqrt(src)<br>
> > > + //<br>
> > > + // (after)<br>
> > > + // v0 = sqrt_noreadmem(src) # native sqrt instruction.<br>
> > > + // if (v0 is a NaN)<br>
> > > + // v1 = sqrt(src) # library call.<br>
> > > + // dst = phi(v0, v1)<br>
> > > + //<br>
> > > +<br>
> > > + // Move all instructions following Call to newly created block<br>
> > > JoinBB.<br>
> > > + // Create phi and replace all uses.<br>
> > > + BasicBlock *JoinBB = llvm::SplitBlock(&CurrBB,<br>
> > > Call->getNextNode(), this);<br>
> > > + IRBuilder<> Builder(JoinBB, JoinBB->begin());<br>
> > > + PHINode *Phi = Builder.CreatePHI(Call->getType(), 2);<br>
> > > + Call->replaceAllUsesWith(Phi);<br>
> > > +<br>
> > > + // Create basic block LibCallBB and insert a call to library<br>
> > > function sqrt.<br>
> > > + BasicBlock *LibCallBB = BasicBlock::Create(CurrBB.getContext(),<br>
> > > "call.sqrt",<br>
> > > + CurrBB.getParent(),<br>
> > > JoinBB);<br>
> > > + Builder.SetInsertPoint(LibCallBB);<br>
> > > + Instruction *LibCall = Call->clone();<br>
> > > + Builder.Insert(LibCall);<br>
> > > + Builder.CreateBr(JoinBB);<br>
> > > +<br>
> > > + // Add attribute "readnone" so that backend can use a native<br>
> > > sqrt<br>
> > > instruction<br>
> > > + // for this call. Insert a FP compare instruction and a<br>
> > > conditional branch<br>
> > > + // at the end of CurrBB.<br>
> > > + Call->addAttribute(AttributeSet::FunctionIndex,<br>
> > > Attribute::ReadNone);<br>
> > > + CurrBB.getTerminator()->eraseFromParent();<br>
> > > + Builder.SetInsertPoint(&CurrBB);<br>
> > > + Value *FCmp = Builder.CreateFCmpOEQ(Call, Call);<br>
> > > + Builder.CreateCondBr(FCmp, JoinBB, LibCallBB);<br>
> > > +<br>
> > > + // Add phi operands.<br>
> > > + Phi->addIncoming(Call, &CurrBB);<br>
> > > + Phi->addIncoming(LibCall, LibCallBB);<br>
> > > +<br>
> > > + BB = JoinBB;<br>
> > > + return true;<br>
> > > +}<br>
> > ><br>
> > > Modified: llvm/trunk/lib/Target/Mips/MipsTargetMachine.cpp<br>
> > > URL:<br>
> > > <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/Mips/MipsTargetMachine.cpp?rev=183802&r1=183801&r2=183802&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/Mips/MipsTargetMachine.cpp?rev=183802&r1=183801&r2=183802&view=diff</a><br>



> > > ==============================================================================<br>
> > > --- llvm/trunk/lib/Target/Mips/MipsTargetMachine.cpp (original)<br>
> > > +++ llvm/trunk/lib/Target/Mips/MipsTargetMachine.cpp Tue Jun 11<br>
> > > 17:21:44 2013<br>
> > > @@ -160,6 +160,7 @@ void MipsPassConfig::addIRPasses() {<br>
> > > addPass(createMipsOs16(getMipsTargetMachine()));<br>
> > > if (getMipsSubtarget().inMips16HardFloat())<br>
> > > addPass(createMips16HardFloat(getMipsTargetMachine()));<br>
> > > +<br>
> > > addPass(createMipsOptimizeMathLibCalls(getMipsTargetMachine()));<br>
> > > }<br>
> > > // Install an instruction selector pass using<br>
> > > // the ISelDag to gen Mips code.<br>
> > ><br>
> > > Added: llvm/trunk/test/CodeGen/Mips/optimize-fp-math.ll<br>
> > > URL:<br>
> > > <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/Mips/optimize-fp-math.ll?rev=183802&view=auto" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/Mips/optimize-fp-math.ll?rev=183802&view=auto</a><br>



> > > ==============================================================================<br>
> > > --- llvm/trunk/test/CodeGen/Mips/optimize-fp-math.ll (added)<br>
> > > +++ llvm/trunk/test/CodeGen/Mips/optimize-fp-math.ll Tue Jun 11<br>
> > > 17:21:44 2013<br>
> > > @@ -0,0 +1,32 @@<br>
> > > +; RUN: llc -march=mipsel < %s | FileCheck %s -check-prefix=32<br>
> > > +; RUN: llc -march=mips64el -mcpu=mips64 < %s | FileCheck %s<br>
> > > -check-prefix=64<br>
> > > +<br>
> > > +; 32: test_sqrtf_float_:<br>
> > > +; 32: sqrt.s $f[[R0:[0-9]+]], $f{{[0-9]+}}<br>
> > > +; 32: c.un.s $f[[R0]], $f[[R0]]<br>
> > > +; 64: test_sqrtf_float_:<br>
> > > +; 64: sqrt.s $f[[R0:[0-9]+]], $f{{[0-9]+}}<br>
> > > +; 64: c.un.s $f[[R0]], $f[[R0]]<br>
> > > +<br>
> > > +define float @test_sqrtf_float_(float %a) {<br>
> > > +entry:<br>
> > > + %call = tail call float @sqrtf(float %a)<br>
> > > + ret float %call<br>
> > > +}<br>
> > > +<br>
> > > +declare float @sqrtf(float)<br>
> > > +<br>
> > > +; 32: test_sqrt_double_:<br>
> > > +; 32: sqrt.d $f[[R0:[0-9]+]], $f{{[0-9]+}}<br>
> > > +; 32: c.un.d $f[[R0]], $f[[R0]]<br>
> > > +; 64: test_sqrt_double_:<br>
> > > +; 64: sqrt.d $f[[R0:[0-9]+]], $f{{[0-9]+}}<br>
> > > +; 64: c.un.d $f[[R0]], $f[[R0]]<br>
> > > +<br>
> > > +define double @test_sqrt_double_(double %a) {<br>
> > > +entry:<br>
> > > + %call = tail call double @sqrt(double %a)<br>
> > > + ret double %call<br>
> > > +}<br>
> > > +<br>
> > > +declare double @sqrt(double)<br>
> > ><br>
> > ><br>
> > > _______________________________________________<br>
> > > llvm-commits mailing list<br>
> > > <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
> > > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
> > ><br>
> > _______________________________________________<br>
> > llvm-commits mailing list<br>
> > <a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
> > <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
> ><br>
> ><br>
><br>
><br>
</div></div></blockquote></div><br></div></div></div></div></div>
</blockquote></div><br></div>