[llvm] r213815 - SimplifyCFG: fix a bug in switch to table conversion

[Manman Ren]

On Thu, Jul 24, 2014 at 11:49 AM, Hans Wennborg <hans at chromium.org> wrote:

> On Wed, Jul 23, 2014 at 4:13 PM, Manman Ren <manman.ren at gmail.com> wrote:
> > Author: mren
> > Date: Wed Jul 23 18:13:23 2014
> > New Revision: 213815
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=213815&view=rev
> > Log:
> > SimplifyCFG: fix a bug in switch to table conversion
> >
> > We use gep to access the global array "switch.table", and the table index
> > should be treated as unsigned. When the highest bit is 1, this commit
> > zero-extends the index to an integer type with larger size.
>
> Thanks for catching and fixing this! Maybe it should get merged to the
> 3.5 branch?
>
> The patch looks good; I only have a few suggestions inline below.
>
> > -Value *SwitchLookupTable::BuildLookup(Value *Index, IRBuilder<>
> &Builder) {
> > +Value *SwitchLookupTable::BuildLookup(Value *Index, uint64_t TableSize,
> > +                                      IRBuilder<> &Builder) {
>
> It feels wrong having to pass the TableSize here. SwitchLookupTable
> should know its own size.
>
> >    switch (Kind) {
> >      case SingleValueKind:
> >        return SingleValue;
> > @@ -3624,6 +3625,14 @@ Value *SwitchLookupTable::BuildLookup(Va
> >                                   "switch.masked");
> >      }
> >      case ArrayKind: {
>
> We can get the table size from the Array member:
>
> uint64_t TableSize =
> cast<Constant>(Array->getInitializer())->getType()->getArrayNumElements();


> (Or we could even store it as a new ArraySize member when we construct
> the Array in the SwitchLookupTable constructor.)
>
> > +      // Make sure the table index will not overflow when treated as
> signed.
> > +      if (IntegerType *IT = dyn_cast<IntegerType>(Index->getType()))
>
> The index is always of integer type, so no need for dyn_cast and if here.
>

>From the language ref, it should be integer type, the verifier does not
verify that, and I just found out that the parser does check that.

In r213895.

Thanks,
Manman


> Cheers,
> Hans
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140724/4d84c188/attachment.html>