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

[Sean Silva]

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
>