[PATCH] TableGen minor refactoring: AsmWriterEmitter and CodeGenDAGPatterns

[Quentin Colombet]

Hi Ahmed,

Good to see you back on the mailing list!

I have looked only at the first patch at the moment.

Here are my remarks:
* I do not understand this comment.
-/// implementation.
+/// implementation. Destroys AsmWriterInst information.

You mean that the information hold in AsmWriterEmitter::Instructions is destroyed, right?

* What is the point of this refactoring?
I completely missed the point of this first refactoring.
You avoid the cost of building a CodeGenTarget several times, which is nice, but I do not see the benefits for the vector of AsmWriterInst.
This does not seems useful either for futur reference, as the field is private and there is no accessors.

Thanks,
-Quentin

On Oct 9, 2013, at 12:56 PM, Ahmed Bougacha <ahmed.bougacha at gmail.com> wrote:

> Hello everyone,
> 
> Here are 2 patches that prepare for upcoming additions. There
> shouldn't be any functionality change.
> Thanks for reviewing!
> 
> - Ahmed
> 
> ----
> 
>    TableGen: Refactor DAG patterns to enable parsing one pattern at a time.
> 
> utils/TableGen/CodeGenDAGPatterns.cpp | 112
> utils/TableGen/CodeGenDAGPatterns.h   |   5
> 2 files changed, 66 insertions(+), 51 deletions(-)
> 
> ----
> 
>    TableGen: Refactor AsmWriterEmitter to keep AsmWriterInsts.
> 
>    These used to be referenced by the CGI->AWI map (in AsmWriterEmitter), but
>    stored in a vector local to EmitPrintInstruction. Move the vector to
>    AsmWriterEmitter too.
> 
> utils/TableGen/AsmWriterEmitter.cpp | 51
> 1 file changed, 25 insertions(+), 26 deletions(-)
> <0001-TableGen-Refactor-AsmWriterEmitter-to-keep-AsmWriter.patch><0002-TableGen-Refactor-DAG-patterns-to-enable-parsing-one.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131014/8cf9e3fa/attachment.html>