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

[Sean Silva]

Hi Jyotsna, this patch addresses the things I brought up. The parts
that I have been reviewing LGTM. Please wait for Jakob to get a chance
to review.

+// CodeGenMapTable parses this map and generates a table in XXXGenInstrInfo.inc
+// file that contains the instructions modeling this relationship. This table
+// is defined in the function
+// "uint16_t getPredOpcode(uint16_t Opcode, enum PredSense inPredSense)"

it looks like the return type here didn't get updated to int16_t.

-- Sean Silva

On Fri, Oct 19, 2012 at 12:32 PM, Jyotsna Verma <jverma at codeaurora.org> wrote:
> 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
>> >