[llvm-commits] working on generalized vector intrinsics

[Chris Lattner]

On Jul 16, 2007, at 9:01 AM, Dan Gohman wrote:

>> 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.
>
> For me, the changes to ctlz and friends are more important than bswap,
> so I'll take a look at having the bc and .ll readers autoupgrade  
> these.

Ok, sounds good!

>> 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?
>
> Itanium, PowerPC, Alpha, SSE4, and various Cray architectures all have
> a ctpop that returns a result of the same size as the operand. This  
> can
> mostly be papered over with codegen peeps, but do you want LLVM to
> follow what appears to be a consensus among diverse hardware, or  
> follow
> the "In C functions should return int because that's how implicit
> declaration works" convention? ;-)

For integers at least, I'm not that concerned.  Have you noticed a  
case where it caused inefficient codegen?  We have similar problems  
with shifts and other operators: for shifts, the shift amount  
sometimes follows the type of the shifted value, sometimes it's fixed  
(e.g. to i8 on x86).

On RISC architectures, often you only have (e.g.) 32-bit registers,  
in which case, an i8 ctlz has issues regardless of whether the result  
is i32 or i8. :)

All that said, I agree with you that it is more natural for these  
operators to return the same type as their input.  I'm just curious  
if you've actually seen a problem.

>>> "The return type must match the argument type."
>>
>> Oops, that should be fixed :(
>
> Or the intrinsics themselves should be fixed :).

Yep, either way works :)

>> ===================================================================
>> --- 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?
>
> I'm not aware of any places that are wrong at the moment, but I've got
> more testing to do in general.

Ok

>> Does the generated type verifier should check for and reject things
>> like "llvm.cos.i32"?
>
> Yes, because it distinguishes between iAny and fAny, and cos uses  
> fAny.

Ok.

>> +  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?
>
> That code is used when a CopyFromReg is an operand of a vector  
> intrinsic
> that is scalarized. I'm not sure this is the right approach though.  
> I'll
> be reevaluating it as I proceed.

Sounds good, thanks Dan!

-Chris