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

[Jyotsna Verma]

Jakob/Sean,

Please find attached the patch with the fix suggested by Sean.

Thanks,
Jyotsna	

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


> -----Original Message-----
> From: Sean Silva [mailto:silvas at purdue.edu]
> Sent: Thursday, October 18, 2012 7:05 PM
> To: Jyotsna Verma
> Cc: Jakob Stoklund Olesen; llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm-commits] [PATCH] TableGen backend support to express
> relations between instruction
> 
> Thanks Jyotsna, the documentation looks great. A couple tiny nits about
the
> documentation and a comment at the end about the code:
> 
> +TableGen uses the above relationship model to emit relation table that
> +maps non-predicated instructions with their predicated forms. It also
> +outputs an interface function ``short getPredOpcode(unsigned Opcode,
> +bool PredSense)`` to query the table. Here, Function ``getPredOpcode``
> +takes two arguments, opcode of the
> 
> It looks like this description is out of date with the header comment in
> utils/TableGen/CodeGenMapTable.cpp. You may want to move the
> description of the signature to the beginning of the example, since this
> signature is the "goal" of the instruction mapping; otherwise the reader
will
> be kind of lost until they reach the end.
> 
> +instruction and returns its predicated true or flase form depending on
> +some
> 
> flase -> false
> 
> 
> 
> +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.
> 
> -- Sean Silva
> 
> On Thu, Oct 18, 2012 at 4:22 PM, Jyotsna Verma <jverma at codeaurora.org>
> wrote:
> > Jakob,
> >
> > This patch addresses all your review comments. Please let me know if
> > it's good to commit.
> >
> > Sean,
> >
> > I have moved documentation into its own file,
> > HowToUseInstrMappings.rst and I have added a link to it from
> > WritingLLVMBackend.html file. I have restructured the document to
> > incorporate some of your suggestions. Please let me know if there are
> > any other suggestions. However, I would prefer to address any major
> > document related issues in a separate patch instead of holding the
entire
> feature.
> >
> > Thanks,
> > Jyotsna
> >
> > --
> > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> > hosted by The Linux Foundation
> >
> >
> >> -----Original Message-----
> >> From: Jakob Stoklund Olesen [mailto:stoklund at 2pi.dk]
> >> Sent: Friday, October 12, 2012 6:53 PM
> >> To: Jyotsna Verma
> >> Cc: 'Sean Silva'; llvm-commits at cs.uiuc.edu
> >> Subject: Re: [llvm-commits] [PATCH] TableGen backend support to
> >> express relations between instruction
> >>
> >>
> >> On Oct 11, 2012, at 8:35 AM, Jyotsna Verma <jverma at codeaurora.org>
> >> wrote:
> >>
> >> > Hello Jakob/Sean,
> >> >
> >> > Here is the updated patch which addresses your review comments.
> >> > Please take a look and let me know if you have any other suggestions.
> >>
> >> Please use uint16_t for opcodes instead of 'short' everywhere.
> >>
> >> +typedef std::map<std::string, std::vector<Record*> > InstrRelMapTy;
> >>
> >> It's not clear why you're keying this map on instruction names
> >> instead of using the much more efficient DenseMap<Record*,.>.
> >>
> >> +// Compare operator used by RowInstrMap to order keys that are
> >> +vectors of Init // pointers.
> >> +struct InstrMapComp {
> >> +  bool operator() (const std::vector<Init*>& lhs,
> >> +                   const std::vector<Init*>& rhs) const {
> >>
> >> Why is this needed?
> >>
> >> +void MapTableEmitter::buildRowInstrMap(raw_ostream &OS) {
> >>
> >> What's the raw_ostream for?
> >>
> >> +    RowInstrMapTy::iterator II, IE;
> >> +    II =  RowInstrMap.find(KeyValue);
> >> +    IE = RowInstrMap.end();
> >> +    if (II == IE) {
> >> +      InstrList.push_back(CurInstr);
> >> +      std::pair<std::vector<Init*>, std::vector<Record*> >
> >> +                                              KeyValuePair(KeyValue,
> > InstrList);
> >> +      RowInstrMap.insert(KeyValuePair);
> >> +    }
> >> +    else {
> >> +      II->second.push_back(CurInstr);
> >> +    }
> >>
> >> RowInstrMap[KeyValue].push_back(CurInstr);
> >>
> >> +//===---------------------------------------------------------------
> >> +---
> >> +----===//
> >> +// Output two-level relation tables.
> >> +
> >>
> >> I'm rather skeptical about the compile time improvements you can get
> >> with this compared to a simple binary search.
> >>
> >> Please save this for a later patch, and include some compile time
> >> measurements so we know it is worth the extra complexity.
> >>
> >> +  OS << "short "<<InstrMapDesc.getName() << "Table[]["<< NumCol+1
> <<
> >> + "] = {\n";
> >>
> >> Tables are constant.
> >>
> >> Please emit just a single function per mapping. The tables can be
> >> local to
> > the
> >> function, and the binary search algorithm should be duplicated so it
> > doesn't
> >> need to deal with dynamic row sizes.
> >>
> >> /jakob
> >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-TableGen-support-to-create-relationship-maps-bet.patch
Type: application/octet-stream
Size: 39715 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121019/8cc32679/attachment.obj>