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 _______________________________________________ llvm-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
