[PATCH] TableGen minor refactoring: AsmWriterEmitter and CodeGenDAGPatterns

[Ahmed Bougacha]

On Tue, Oct 15, 2013 at 10:32 AM, Quentin Colombet <qcolombet at apple.com> wrote:
> 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.

r19352[56] with the changes, thanks!

- Ahmed

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