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

Moving discussion to llvm-commits now that I have a more developed
implementation:
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20131111/195401.
html

> -----Original Message-----
> From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at
cs.uiuc.edu] On
> Behalf Of David Peixotto
> Sent: Tuesday, November 12, 2013 11:09 AM
> To: 'Amara Emerson'
> Cc: llvmdev at cs.uiuc.edu
> Subject: Re: [LLVMdev] Implementing the ldr pseudo instruction in ARM
> integrated assembler
> 
> Hi Amara,
> 
> Thanks for your suggestions. I have made the changes you suggested and
> added a new test to check that we print an error when parsing a non-ldr
> mnemonic with an operand containing `=`. The updated patch is attached.
> 
> -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
> 
> 
> 
> > -----Original Message-----
> > From: Amara Emerson [mailto:amara.emerson at arm.com]
> > Sent: Tuesday, November 12, 2013 5:43 AM
> > To: 'David Peixotto'
> > Cc: llvmdev at cs.uiuc.edu
> > Subject: RE: [LLVMdev] Implementing the ldr pseudo instruction in ARM
> > integrated assembler
> >
> > Hi David,
> >
> > Thanks for your efforts here. I have a few comments on your patch,
> > although I realise it's still a work in progress.
> >
> > +class ConstantPool {
> > +  MCSymbol *Label;
> > +  typedef std::vector<const MCExpr*> EntryVecTy;
> >
> > Use a SmallVector here?
> >
> > +  MCSymbol *getLabel() {return Label;}  size_t getNumEntries()
> > + {return Entries.size();}  const MCExpr *getEntry(size_t Num) {return
> > + Entries[Num];}
> > These can be const.
> >
> > +  int64_t ConstantPoolCounter;
> > This can be unsigned.
> >
> > +  case AsmToken::Equal: {
> > +    const MCSection *Section > >
getParser().getStreamer().getCurrentSection().first;
> > +    assert(Section);
> > We should probably check here that the mnemonic is actually
'ldr', e.g.
> > see the AsmToken::Identifier case.
> >
> > +@ RUN: clang -target arm -integrated-as -c %s -o %t1 2> %t2; echo
"ok"
> > +@ RUN: cat %t2 | FileCheck %s
> > Clang tests shouldn't be in the LLVM regression suite. Use llvm-mc
> > instead for assembling.
> >
> > I'm not very familiar with the code around the asm parser, so I
expect
> > more detailed comments from others to follow.
> >
> > Cheers,
> > Amara
> >
> >

Hi David,

Maybe I’m just blind, but where’s the code to handle the .ltorg directive? Is
that a separate patch, maybe? Without that, this is not going to be usable in
any circumstance using subsections-via-symbols.

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

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

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

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


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

These tests are very linux specific, including names and relocation type
information. What happens when compiling on or for other platforms?


-Jim

On Nov 15, 2013, at 6:05 PM, David Peixotto <dpeixott at codeaurora.org>
wrote:
> Moving discussion to llvm-commits now that I have a more developed
> implementation:
>
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20131111/195401.
> html
> 
> 
>> -----Original Message-----
>> From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at
cs.uiuc.edu] On
>> Behalf Of David Peixotto
>> Sent: Tuesday, November 12, 2013 11:09 AM
>> To: 'Amara Emerson'
>> Cc: llvmdev at cs.uiuc.edu
>> Subject: Re: [LLVMdev] Implementing the ldr pseudo instruction in ARM
>> integrated assembler
>> 
>> Hi Amara,
>> 
>> Thanks for your suggestions. I have made the changes you suggested and
>> added a new test to check that we print an error when parsing a non-ldr
>> mnemonic with an operand containing `=`. The updated patch is attached.
>> 
>> -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>> hosted by The Linux Foundation
>> 
>> 
>> 
>>> -----Original Message-----
>>> From: Amara Emerson [mailto:amara.emerson at arm.com]
>>> Sent: Tuesday, November 12, 2013 5:43 AM
>>> To: 'David Peixotto'
>>> Cc: llvmdev at cs.uiuc.edu
>>> Subject: RE: [LLVMdev] Implementing the ldr pseudo instruction in
ARM
>>> integrated assembler
>>> 
>>> Hi David,
>>> 
>>> Thanks for your efforts here. I have a few comments on your patch,
>>> although I realise it's still a work in progress.
>>> 
>>> +class ConstantPool {
>>> +  MCSymbol *Label;
>>> +  typedef std::vector<const MCExpr*> EntryVecTy;
>>> 
>>> Use a SmallVector here?
>>> 
>>> +  MCSymbol *getLabel() {return Label;}  size_t getNumEntries()
>>> + {return Entries.size();}  const MCExpr *getEntry(size_t Num)
{return
>>> + Entries[Num];}
>>> These can be const.
>>> 
>>> +  int64_t ConstantPoolCounter;
>>> This can be unsigned.
>>> 
>>> +  case AsmToken::Equal: {
>>> +    const MCSection *Section >>>
getParser().getStreamer().getCurrentSection().first;
>>> +    assert(Section);
>>> We should probably check here that the mnemonic is actually
'ldr', e.g.
>>> see the AsmToken::Identifier case.
>>> 
>>> +@ RUN: clang -target arm -integrated-as -c %s -o %t1 2> %t2;
echo "ok"
>>> +@ RUN: cat %t2 | FileCheck %s
>>> Clang tests shouldn't be in the LLVM regression suite. Use
llvm-mc
>>> instead for assembling.
>>> 
>>> I'm not very familiar with the code around the asm parser, so I
expect
>>> more detailed comments from others to follow.
>>> 
>>> Cheers,
>>> Amara
>>> 
>>> 
> 
> 
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

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>