[llvm-commits] [PATCH] TableGen backend support to express relations between instruction

[Jyotsna Verma]

Hello Sean,

In the previous email, I forgot to address your concern for returning -1
from the function. I don't think we can assert here because the client may
not know if the instruction has a particular form it's interested in. Client
will have to check for error (-1) unless there is another mechanism (maybe
through TSFlags) that checks if a particular transition is supported. 

Thanks,
Jyotsna
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by
The Linux Foundation


> -----Original Message-----
> From: Jyotsna Verma [mailto:jverma at codeaurora.org]
> Sent: Thursday, October 18, 2012 11:23 PM
> To: 'Sean Silva'
> Cc: 'Jakob Stoklund Olesen'; 'llvm-commits at cs.uiuc.edu'
> Subject: RE: [llvm-commits] [PATCH] TableGen backend support to express
> relations between instruction
> 
> Hello Sean,
> 
> Thanks for reviewing the patch.
> Problem with this code is 'start, mid and end' being 'unsigned'. Had they
> been declared 'int', there wouldn't be any overflow problem on 'end = mid-
> 1'. I'll change 'mid = (start+end)/2' to 'mid=start+(end-start)/2' for the
valid
> reasons you've pointed out.
> 
> Thanks,
> Jyotsna
> 
> > +void MapTableEmitter::emitBinSearch(raw_ostream &OS, unsigned
> > +TableSize) {
> > +  OS << "  unsigned mid;\n";
> > +  OS << "  unsigned start = 0;\n";
> > +  OS << "  unsigned end = " << TableSize - 1 << ";\n";
> > +  OS << "  while (start <= end) {\n";
> > +  OS << "    mid = (end + start)/2;\n";
> > +  OS << "    if (Opcode == " << InstrMapDesc.getName() <<
"Table[mid][0])
> > {\n";
> > +  OS << "      break;\n";
> > +  OS << "    }\n";
> > +  OS << "    if (Opcode < " << InstrMapDesc.getName() <<
> > "Table[mid][0])\n";
> > +  OS << "      end = mid - 1;\n";
> > +  OS << "    else\n";
> > +  OS << "      start = mid + 1;\n";
> > +  OS << "  }\n";
> > +  OS << "  if (start > end)\n";
> > +  OS << "    return -1; // Instruction doesn't exist in this
table.\n\n";
> > +}
> >
> > This code is buggy. In `end = mid - 1` could evaluate to UINT_MAX in
> > the case of `TableSize == 2`, since `start` will be initialized to 0,
> > `end` will be initialized to `1`, so `mid` will be `(0+1)/2` which is
> > 0. Then if the given opcode is less than the value in the table, you
> > will perform `end = mid - 1` which gives UINT_MAX.
> >
> > I recommend using half-open ranges with a loop invariant "the element
> > we are looking for, if present, is in the range [start,end)", and the
> > termination condition is `start == end`. Also, `mid = (end + start)/2`
> > is potentially dangerous, since (end+start) could overflow an unsigned
> > and then dividing by 2 will give an undesired result; it is better to
> > do `mid = start + (end - start)/2`. Alternatively, if you know that
> > you can count on <algorithm> in this code, just use std::lower_bound().
> >
> > Also, I'm not sure if this routine is meant to return `-1` as an error
> > code (are clients going to be checking this?). If clients are not
> > expected to check this, then you should probably assert that an entry
was
> found in the table.