[llvm-commits] [llvm] r93448 - /llvm/trunk/lib/Analysis/InlineCost.cpp

[Eric Christopher]

On Jan 15, 2010, at 12:10 AM, Duncan Sands wrote:

> Hi Eric, if you check the code in SelectionDAGBuilder.cpp, you will see that
> it checks more than just the name before deciding whether to change into a
> SDag node.  For example, here is the code for sqrt:
> 
>      } else if (Name == "sqrt" || Name == "sqrtf" || Name == "sqrtl") {
>        if (I.getNumOperands() == 2 &&   // Basic sanity checks.
>            I.getOperand(1)->getType()->isFloatingPoint() &&
>            I.getType() == I.getOperand(1)->getType() &&
>            I.onlyReadsMemory()) {
>          SDValue Tmp = getValue(I.getOperand(1));
>          setValue(&I, DAG.getNode(ISD::FSQRT, getCurDebugLoc(),
>                                   Tmp.getValueType(), Tmp));
>          return;
>        }
> 
> Notice how it checks I.onlyReadsMemory to see if sqrt writes errno or not?
> Also, your code (and that in SDag) suffers from not being turn-offable.
> How about factoring the checks out of SelectionDAGBuilder.cpp and reusing
> them here?

Yes, I saw that.  Largely I wasn't as concerned with that since, in general, I don't think it's still much of an issue even still since most of those functions are still going to be lowered or optimized.  That said, I'm not wedded to not doing it either. :) The errno checks are probably a good idea, but the sanity checks really aren't necessary.

I don't think the code should be able to be turned off - I can see doing it in SelectionDAGBuilder since it's an "optimization", but we're already in an opt pass here.

Sorry for the wishy washy email, but unfortunately that was my general thought while looking at it anyhow ;)

-eric