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

[David Peixotto]

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.

> Maybe I'm just blind, but where's the code to handle the .ltorg directive?

It is implemented in patch 0003 in this email.

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

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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-a-finishParse-callback-to-the-targer-asm-parser.patch
Type: application/octet-stream
Size: 1585 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20131217/48eb4e06/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Implement-the-ldr-pseudo-opcode-for-ARM-assembly.patch
Type: application/octet-stream
Size: 24109 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20131217/48eb4e06/attachment-0001.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Implement-the-.ltorg-directive-for-ARM-assembly.patch
Type: application/octet-stream
Size: 8357 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20131217/48eb4e06/attachment-0002.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: squashed.patch
Type: application/octet-stream
Size: 33102 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20131217/48eb4e06/attachment-0003.obj>