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

[Jim Grosbach]

On Dec 17, 2013, at 11:33 AM, 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.

Responses to a few specific questions below, and I’ll follow up on the new patch separately.

>> Maybe I'm just blind, but where's the code to handle the .ltorg directive?
> 
> It is implemented in patch 0003 in this email.

Excellent.


> 
>> +typedef std::map<const MCSection *, ConstantPool> ConstantPoolMapTy;
>> 
>> This feels odd to me. Can you elaborate a bit more on the data structure
>> choices?? I would have expected constants to be grouped together
>> explicitly by section and then a nested loop over those sections instead.
>> As-is, won't this potentially cause lots of switching back and forth
>> between sections at the end of assembly?
> 
> I just added this comment to the code to try and address your question:
> 
> // Map type used to keep track of per-Section constant pools used by the
> // ldr-pseudo opcode. The map associates a section to its constant pool. The
> // constant pool is a vector of (label, value) pairs. When the ldr
> // pseudo is parsed we insert a new (label, value) pair into the constant
> pool
> // for the current section and add MCSymbolRefExpr to the new label as
> // an opcode to the ldr. After we have parsed all the user input we
> // output the (label, value) pairs in each constant pool at the end of the
> // section.
> typedef std::map<const MCSection *, ConstantPool> ConstantPoolMapTy;
> 
> Basically it groups the constants by section and writes them out at the end.
> It will have to switch sections at the end of the assembly once per section
> that has a constant pool.
> 
> 
>> +      MCSymbol *Label =
>> +        getContext().GetOrCreateSymbol("$cp." +
>> + Twine(ConstantPoolCounter++));
>> 
>> As previously mentioned, this sort of label name isn't portable. Whether
>> '$' is legal isn't guaranteed, and there's no assembler-local label
>> prefix.
> 
> This was fixed in a later patch by using the CreateTempSymbol() API in
> MCContext.
> 
>> +    ConstantPoolMapTy::iterator Cp = ConstantPools.find(Section);
>> 
>> Minor detail in various places in the code. Acronyms should be
>> capitalized, so "CP" instead of "Cp". Even better would be spelled out
>> names if they're short enough to make sense in the context of the code.
> 
> I will clean that up when I rebase the changes.
> 
>> +void ARMAsmParser::finishParse() {
>> +  for (ConstantPoolMapTy::iterator CpI = ConstantPools.begin(), CpE =
>> ConstantPools.end(); CpI != CpE; ++CpI) {
>> +    const MCSection *Section = CpI->first;
>> +    ConstantPool &CP = CpI->second;
>> +    MCStreamer &Streamer = getParser().getStreamer();
>> +    Streamer.SwitchSection(Section);
>> +    Streamer.EmitLabel(CP.getLabel());
>> +    for (size_t I = 0; I < CP.getNumEntries(); ++I)
>> +      Streamer.EmitValue(CP.getEntry(I), 4);
>> +  }
>> +}
>> 
>> This is missing data-in-code indicators (see EmitDataRegion()).
> 
> I was not aware of that API, thanks for pointing it out. I added the calls
> in the latest patch. See the ConstantPool::emitEntries() method.
> 
>> + at RUN: clang -target arm -integrated-as -c %s -o %t
>> + at RUN: llvm-objdump -d %t | FileCheck %s
>> + at RUN: llvm-objdump -r %t | FileCheck --check-prefix=RELOC %s
>> 
>> MC tests should not call clang. There is no guarantee that clang will be
>> present on the system or being built in the current LLVM build tree. Use
>> llvm-mc instead. 
> 
> That was fixed in a later patch.
> 
>> 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.

> 
>> These tests are very linux specific, including names and relocation type
>> information. What happens when compiling on or for other platforms?
> 
> Please see the darwin tests in the latest patch.

Will do, and thanks again.

-Jim

> 
> <0001-Add-a-finishParse-callback-to-the-targer-asm-parser.patch><0002-Implement-the-ldr-pseudo-opcode-for-ARM-assembly.patch><0003-Implement-the-.ltorg-directive-for-ARM-assembly.patch><squashed.patch>