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

[Jyotsna Verma]

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: 39612 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20121018/cdb9772a/attachment.obj>