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

[Chandler Carruth]

On Mon, Dec 12, 2011 at 1:14 AM, Duncan Sands <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!
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20111212/650a5091/attachment.html>