llvm dev - Dec 2013 - [LLVMdev] Implementing the ldr pseudo instruction in ARM integrated assembler

> > Hi Jim,
> >
> > Thanks for the review. It seems like you were looking at an old patch.
> > I've attached the latest patches to this email (and a squashed
version
> > of all three for easy reading). I believe many of your concerns were
> > addressed. See below for a detailed response.
> >
> Ack! Sorry about that. That would certainly explain some of my confusion.
> I suspect some (mis)threading of emails got in the way. Thanks again for
> your patience working through this.
Yes there were a number of patches spread out over a period of time. I am
close to getting permission to use phabricator which should help alleviate
this problem in the future.
> >> Likewise, there shouldn't be any need to output an object
file. Just
> >> checking the .s output from the asm streamer should be sufficient
> >> here.
> >
> > I deliberately used an object file test because I wanted to make sure
> > that the correct offsets and relocations were being generated.
> > Additionally, some error tests require object code emission (to find
> > out the constant pool offset is too far away). If you feel strongly
> > that they should be converted to asm streamer tests I will do it.
> 
> A bit of both. I feel strongly that there should be tests that don't
emit
> to object files, but do agree that there's value to checking object as
> well (for the range checking in particular). In particular, the asm
> streamer tests can probably be platform independent, or at least close.
> The object file tests obviously cannot. For the relocations, is there
> something specific about the code generated here that tests those in ways
> that existing relocation tests don't cover? I would have expected that
> part to be totally generic, as the pseudo is just a syntactic convenience
> after all and not expressing anything that can't already be written in
a
> .s file without the construct.
I wrote the tests before I did the actual implementation and I thought I
would be generating the relocations myself. The implementation turned out to
be much simpler than I imagined and I was able to lean on the existing
support for generating relocations and fixups. I expect all these
relocations would be covered by other tests. 

I think we still would need separate tests for darwin/linux because the
syntax for creating sections (for example) is different.

One option is to leave the object test for the errors (out of range fixup)
and switch to asm tests for the others.

On Dec 17, 2013, at 12:00 PM, David Peixotto <dpeixott at codeaurora.org>
wrote:
>>> Hi Jim,
>>> 
>>> Thanks for the review. It seems like you were looking at an old
patch.
>>> I've attached the latest patches to this email (and a squashed
version
>>> of all three for easy reading). I believe many of your concerns
were
>>> addressed. See below for a detailed response.
>>> 
>> Ack! Sorry about that. That would certainly explain some of my
confusion.
>> I suspect some (mis)threading of emails got in the way. Thanks again
for
>> your patience working through this.
> 
> Yes there were a number of patches spread out over a period of time. I am
> close to getting permission to use phabricator which should help alleviate
> this problem in the future.
> 
>>>> Likewise, there shouldn't be any need to output an object
file. Just
>>>> checking the .s output from the asm streamer should be
sufficient
>>>> here.
>>> 
>>> I deliberately used an object file test because I wanted to make
sure
>>> that the correct offsets and relocations were being generated.
>>> Additionally, some error tests require object code emission (to
find
>>> out the constant pool offset is too far away). If you feel strongly
>>> that they should be converted to asm streamer tests I will do it.
>> 
>> A bit of both. I feel strongly that there should be tests that
don't emit
>> to object files, but do agree that there's value to checking object
as
>> well (for the range checking in particular). In particular, the asm
>> streamer tests can probably be platform independent, or at least close.
>> The object file tests obviously cannot. For the relocations, is there
>> something specific about the code generated here that tests those in
ways
>> that existing relocation tests don't cover? I would have expected
that
>> part to be totally generic, as the pseudo is just a syntactic
convenience
>> after all and not expressing anything that can't already be written
in a
>> .s file without the construct.
> 
> I wrote the tests before I did the actual implementation and I thought I
> would be generating the relocations myself. The implementation turned out
to
> be much simpler than I imagined and I was able to lean on the existing
> support for generating relocations and fixups. I expect all these
> relocations would be covered by other tests. 
> 
Makes sense. FWIW, I’m pleasantly stunned at how small the actual patches are. I
was expecting it to need more plumbing. This is most excellent.
> I think we still would need separate tests for darwin/linux because the
> syntax for creating sections (for example) is different.
Fair. Can just be multiple RUN lines in the same test file?
> One option is to leave the object test for the errors (out of range fixup)
> and switch to asm tests for the others.
This sounds like a good approach to me.

Now that I’m looking at the right code, I don’t see anything more that needs
doing than what’s already been discussed. This LGTM with those changes.

Regards,
  Jim

> > I wrote the tests before I did the actual implementation and I thought
> > I would be generating the relocations myself. The implementation
> > turned out to be much simpler than I imagined and I was able to lean
> > on the existing support for generating relocations and fixups. I
> > expect all these relocations would be covered by other tests.
> >
> 
> Makes sense. FWIW, I'm pleasantly stunned at how small the actual
patches
> are. I was expecting it to need more plumbing. This is most excellent.
Yes I was suprised as well.
> > I think we still would need separate tests for darwin/linux because
> > the syntax for creating sections (for example) is different.
> 
> Fair. Can just be multiple RUN lines in the same test file?
I think it still needs separate files because of the different assembly
input syntax for darwin/linux.

> > One option is to leave the object test for the errors (out of range
> > fixup) and switch to asm tests for the others.
> 
> This sounds like a good approach to me.
> 
> Now that I'm looking at the right code, I don't see anything more
that
> needs doing than what's already been discussed. This LGTM with those
> changes.
Ok, I will make the final changes and commit. Of course any further feedback
can be addressed in a post-commit review.