[PATCH] TableGen minor refactoring: AsmWriterEmitter and CodeGenDAGPatterns

[Quentin Colombet]

Hi Ahmed,

On Oct 14, 2013, at 6:00 PM, Ahmed Bougacha <ahmed.bougacha at gmail.com> wrote:

> On Tue, Oct 15, 2013 at 2:33 AM, Quentin Colombet <qcolombet at apple.com> wrote:
>> 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?
> 
> Yes, the meaning was: it destroys all instances of AsmWriterInst
> (stored in AsmWriterEmitter::Instructions); I'll rephrase that.
Thanks.

> 
>> * 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.
> 
> Two reasons:
> 
> - first, the CGIAWIMap kept pointers to the elements of
> AsmWriterEmitter::Instructions. The former is a member of
> AsmWriterEmitter, the latter is local to
> AsmWriterEmitter::EmitPrintInstruction, so there's a lifetime problem
> there. This isn't a big deal, but isn't great either.
Makes sense.

> 
> - second, this enables another patch I have, which adds a method to
> AsmWriterEmitter that generates a generic printOperand method, based
> on the print methods that were used: this is where the
> AsmWriterEmitter::Instructions vector is useful.
Okay.

> 
> Thanks for reviewing, Quentin!
Regarding the second patch.
It looks good to me with one minor modification:
-
+  /// Parse the Pattern for an instruction, and insert the result in DAGInsts.
+  const DAGInstruction &parseInstructionPattern(
+      CodeGenInstruction &CGI, ListInit *Pattern,
+      std::map<Record *, DAGInstruction, LessRecordByID> &DAGInsts);

You are exposing a “complex” type (std::map<…>). To ease usability, I think it would be nice to have a typedef for it.

Thanks.
Quentin

> 
> - Ahmed
> 
>> 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
>> 
>>