[llvm-commits] PR1297 patch

[Chris Lattner]

In langref:

+<p>This is an overloaded intrinsic. You can use llvm.ctpop on any  
integer bit
+width. Not all targets support all bit widths however.

+  declare i256 @llvm.ctpop.i256(i256 <src>) ; might fail code gen


Please remove comments about codegen limitations.  It's not the  
intrinsics fault that i27 isn't supported by all targets :)

We should make the codegen print a sensible error message, then exit,  
if it sees stuff it doesn't handle, instead of asserting and dying.



In Intrinsics.td, please add the new intrinsics as a separate follow- 
on patch.  These intrinsics will need to be documented in langref,  
etc as a first step (and I'd like to look at the langref changes  
before you add the intrinsics)

In Intrinsics.td, it looks like you didn't choose to make the "count"  
intrinsics always return i32.  As such, you need to update the  
LangRef.html doc to talk about bswap.i32.i32 (etc) and ctpop.i47.i47,  
not bswap.i32 and ctpop.i47.


I didn't follow the logic in the verifier, but it looks like you  
cover the right cases.


In ConstantFolding.cpp, instead of:

    if (Name.substr(0,10) == "llvm.bswap")

Please use:

    if (Name.size() > 11 && !memcmp(&Name[0], "llvm.bswap", 10)

Please delete:
   if (OpTy->getBitWidth() >= 16 && OpTy->getBitWidth() % 16 == 0)
as the verifier checks it.



In InstCombine, drop "if (BitWidth >= 16 && BitWidth % 16 == 0) {",  
which makes BitWidth dead.



In IntrinsicLowering.cpp, note that "LowerCTPOP", "LowerBSWAP", etc,  
don't work with arbitrary width versions of these.  "Future work" is  
fine though.



In llvm-gcc/llvm-convert.cpp, you can eliminate the cast<IntegerType>  
in EmitBuiltinUnaryIntOp.


Overall, this looks quite awesome.  Very nice work, and a great  
solution to variable width intrinsics!

-Chris