[llvm-commits] working on generalized vector intrinsics

[Chris Lattner]

On Jul 13, 2007, at 1:35 PM, Dan Gohman wrote:

> This patch adds a new ValueType for overloading on floating-point  
> types,
> fAny, and reworks some of the overloading framework to allow iAny  
> and fAny
> to be overloaded with vector types.

Nice.

> In its current form it introduces two incompatibilities. That's no  
> problem
> for my own current projects, so I'm interested to hear from people  
> where
> this is a concern.

We can do this, if there is a good reason.  However, doing it means  
that the bc reader and .ll parser need to "autoupgrade" their inputs  
if presented with an old LLVM 2.0 .ll or .bc file.  Unless there is a  
really good reason to change these, I don't think it's worth the  
trouble.  If you're still like to change them, the .ll and .bc  
readers need to be updated to do this.  When this is done, you  
shouldn't have to upgrade the tests in the testsuite :)

However, I like your LLVMMatchType mechanism, and using it for new  
intrinsics is unconditionally good.

To me, I agree that having a single type listed for bswap is nice,  
but I don't think it's worth it to change the return type of ctlz and  
friends.

> It generalizes sqrt and powi, and adds
> a few new intrinsics, sin, cos, exp, log, and pow, which  
> demonstrate the
> new functionality.

Ah, I see why you want this.  Very nice.  When you document the new  
intrinsics, you should say that they differ from the libc functions  
in that it is undefined whether they set errno (for things like pow,  
this does not affect sin/cos).


> The other incompatibility is with ctpop, ctlz, and cttz. Instead of  
> always
> returning i32, these now have a return type which matches their  
> operand type.
> This is what the SelectionDAG framework currently has; this patch  
> makes
> the LLVM intrinsics work the same way. The primary reason for the  
> change is
> to make these intrinsics consistent when they are overloaded with  
> vector
> types, as it is very awkward to make them always return <N x i32>, and
> somewhat awkward if the scalar versions are inconsistent with the  
> vector
> versions. This change would requires a change to llvm-gcc and any  
> other
> front-end that uses these.

Ok.  Is this a theoretical problem or a real problem?  Do you have a  
target that provides hardware support for element-size ctlz/cttz/ 
popcount?

> It's interesting to note that both of these changes reflect the  
> original
> versions of these intrinsics in LLVM, which were subsequently  
> changed to
> accomodate the overloading framework. For example, there is still  
> text in
> the LangRef.html for ctpop and friends that says
> "The return type must match the argument type."

Oops, that should be fixed :(



Specific feedback w.r.t your patch:

I like your solution to eliminate LLVMIntegerType, LLVMVectorType etc.

===================================================================
--- utils/TableGen/CodeGenTarget.cpp	(revision 39829)
+++ utils/TableGen/CodeGenTarget.cpp	(working copy)
@@ -93,7 +95,7 @@
    case MVT::v2f32: return "MVT::v2f32";
    case MVT::v4f32: return "MVT::v4f32";
    case MVT::v2f64: return "MVT::v2f64";
-  case MVT::iPTR:  return "TLI.getPointerTy()";
+  case MVT::iPTR:  return "MVT::iPTR";
    default: assert(0 && "ILLEGAL VALUE TYPE!"); return "";
    }
  }


Do you correctly handle this change in all places?


Does the generated type verifier should check for and reject things  
like "llvm.cos.i32"?


Your patch to llvm-upgrade needs improvement :)


+  case ISD::CopyFromReg:
+    Lo = DAG.getNode(ISD::EXTRACT_SUBVECTOR, NewVT, Op,
+                     DAG.getConstant(0, MVT::i32));
+    Hi = DAG.getNode(ISD::EXTRACT_SUBVECTOR, NewVT, Op,
+                     DAG.getConstant(NewNumElts, MVT::i32));
+    break;

Why does legalize need to split up CopyFromReg nodes?  Previously,  
they were always legal, did something change?


Overall, I really like the spirit of the patch, the only issue is the  
backwards compatibility thing above.

-Chris

ps, as a random aside, every time I see FLOG, I read it as the word,  
not F-LOG ;-)