[LLVMdev] FP Intrinsics

[Chris Lattner]

On Fri, 11 Mar 2005, Morten Ofstad wrote:

> Hello,
>
> I am trying to make the FP intrinsics (abs, sin, cos, sqrt) I've added work 
> with the X86ISelPattern, but I'm having some difficulties understanding what 
> needs to be done.

Cool.  Here are a couple of requests:

1. I don't think we need an "llvm.abs" intrinsic at the llvm level.  This
    can be modeled as a pair of setcc/select instructions.
2. My objection to llvm.abs does not apply to the FP_ABS
    node you added: it is fine.  Additionally, the target-independent
    selection dag machinery should be the one that notices the relevant
    setcc/select pairs at the llvm level to fabricate the FP_ABS node.
3. On X86 at least, sin and cos are not defined over the full numeric
    range.  These instructions are useful for applications like yours, and
    situations where a flag like "-ffast-math" has been provided.  Because
    of this, please name the intrinsics and nodes sin_approx and
    cos_approx.  I don't think that sqrt on the X86 has this limitation,
    so its intrinsic can be named just "llvm.sqrt".
4. Don't forget a doc patch to docs/LangRef.html :-)

> I assume I have to add new nodetypes for the FP 
> instructions to SelectionDAGNodes.h, and make nodes for these in 
> SelectionDAGLowering::visitCall when I find the intrinsic...

This looks good.

> The part I don't quite understand is what to do for targets that don't have 
> these instructions (although I'm only interested in X86 myself, I would like 
> to see these patches in the official LLVM version as it's some work to 
> maintain them)

Ok, sounds good.

> -- for me it would make most sense to lower the intrinsic to a 
> call if it's not supported. However I notice that for other intrinsics 
> (memcpy etc.) this is done in LegalizeDAG where the node is expanded to a 
> call if it's not directly supported for the target.

Yup, this is what we want to do.  There are two places to implement this: 
LegalizeDAG for the SelectionDAG isels, and 
lib/CodeGen/IntrinsicLowering.cpp for other isels.

> To illustrate what I have been doing so far I attach the diff to this mail -- 
> if someone could have a look at it and tell me what needs to be done to get 
> it working I would be very grateful... (Note that if you use the 
> X86ISelSimple it's already working...)

One of the problems that jumps out at me in the pattern version is that 
you have code that looks like this:

+    case Intrinsic::sqrt:
+      setValue(&I, DAG.getNode(ISD::FP_SQRT, MVT::f64,
+                               getValue(I.getOperand(1))));

In your case, you're passing in an float, which is 32-bits.  You probably 
want the type argument for this to be whatever the input type is, not hard 
coded to f64.

-Chris

-- 
http://nondot.org/sabre/
http://llvm.cs.uiuc.edu/