[LLVMdev] Implementing the ldr pseudo instruction in ARM integrated assembler

[Jim Grosbach]

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