[llvm-commits] [llvm] r146357 - in /llvm/trunk: include/llvm/Intrinsics.td lib/Analysis/ConstantFolding.cpp lib/Transforms/Scalar/SimplifyLibCalls.cpp lib/VMCore/AutoUpgrade.cpp

[Duncan Sands]

Hi Chandler, don't listen to that Chris guy, what does he know?! :)  Given the
amount of codegen ugliness (i.e. either duplicating the set of CTTZ etc nodes,
one for each flag version, or modifying a ton of patterns to expect more than
one argument) hopefully Chris will accept the simpler version of your patch in
which there is only the "undef" form of the intrinsics.  Someone can wlays add
a flag later if there proves to be a really compelling need for it such as
codegen finding it too hard to get rid of pointless comparisons of the argument
with zero on platforms with always-well-defined bit counting instructions.

Ciao, Duncan.

On 12/12/11 10:56, Chandler Carruth wrote:
> On Mon, Dec 12, 2011 at 1:14 AM, Duncan Sands <baldrick at free.fr
> <mailto:baldrick at free.fr>> wrote:
>
>     Hi Chandler,
>
>      > Switch llvm.cttz and llvm.ctlz to accept a second i1 parameter which
>      > indicates whether the intrinsic has a defined result for a first
>      > argument equal to zero.
>
>     is this flag really useful, i.e. isn't the undef form enough?  To get the
>     other form you can test the parameter against zero and use a select.  Then
>     codegen can zap the select on targets where the processor instruction does
>     the right thing for an argument of zero.
>
>
> I completely agree. Chris wanted it this way, and although it makes the
> implementation more complex and more work, I didn't feel like arguing. Feel free
> to do so, and if you win, I'll change it. =D
>
>      > --- llvm/trunk/lib/Analysis/ConstantFolding.cpp (original)
>      > +++ llvm/trunk/lib/Analysis/ConstantFolding.cpp Sun Dec 11 22:26:04 2011
>      > @@ -1418,6 +1414,14 @@
>      >             };
>      >             return
>     ConstantStruct::get(cast<StructType>(F->getReturnType()), Ops);
>      >           }
>      > +        case Intrinsic::cttz:
>      > +          // FIXME: This should check for Op2 == 1, and become
>     unreachable if
>
>     Don't you mean "undef" rather than "unreachable"?
>
>
> Yea, sorry, I'll clean up these comments when I start implementing the proper
> behavior for this case.
>
>
>      > +          // Op1 == 0.
>      > +          return ConstantInt::get(Ty, Op1->getValue().countTrailingZeros());
>      > +        case Intrinsic::ctlz:
>      > +          // FIXME: This should check for Op2 == 1, and become
>     unreachable if
>      > +          // Op1 == 0.
>      > +          return ConstantInt::get(Ty, Op1->getValue().countLeadingZeros());
>      >           }
>      >         }
>      >
>      >
>      > --- llvm/trunk/lib/VMCore/AutoUpgrade.cpp (original)
>      > +++ llvm/trunk/lib/VMCore/AutoUpgrade.cpp Sun Dec 11 22:26:04 2011
>      > @@ -40,7 +40,20 @@
>      >
>      >     switch (Name[0]) {
>      >     default: break;
>      > -  // SOMEDAY: Add some.
>      > +  case 'c': {
>      > +    Type *Tys[] = { F->arg_begin()->getType() };
>
>     If one day there is an llvm intrinsic with a name starting with "c" with no
>     arguments, won't this explode?
>
>
> Yikes, good catch. Will fix. =]
>
> Thanks for the review!