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

In an earlier email[1] I proposed adding support for the ldr
pseud-instruction to the ARM integrated assembler. After some discussion the
overall consensus seemed to be that it was worth adding. One concern was
that we needed to have adequate testing. I promised to provide more details
on what the behavior should be and provide some tests before starting the
implementation. The FileCheck-ified tests are attached to this email. They
currently pass when using gcc to assemble the test.

Here are the specific behaviors that are checked in the test:

1. Check that constants that fit into imm12 are converted to mov
    ldr r0, =0x3
2. Check that large constants are converted to ldr from constant pool
    ldr r0, =0x103
3. Duplicate constants should be merged to the same constant pool location
    ldr r0, =0x103
    ...
    ldr r0, =0x103
4. A section defined in multiple pieces is merged and uses a single constant
pool
    For example,
    
    .section e, "ax", %progbits
      f6:
        ldr r0, =0x103
    .section f, "ax", %progbits
      f7: add r0, r0, r0
    .section e, "ax", %progbits
      f8:
        add r0, r0, r0
        ldr r0, =0x105

    should only produce one constant pool for section e.

5. Check that symbols can be loaded using ldr pseudo
  ldr r0, =foo

6. Check that ldr pseudo can be used with expressions
  ldr r0, =0x101+6
  ldr r0, =bar+4

7. Check that it is used correctly in a macro
    .macro useit_in_a_macro
      ldr r0, =0x101
      ldr r0, =bar
    .endm
    .section k, "ax", %progbits
    f14:
      useit_in_a_macro

8. Check that an error is issued when the constant pool would be placed too
far away
  ldr r0, =0x101
  @... lots of instructions
  Error: invalid literal constant: pool needs to be closer

I have not yet looked into the code to see where/how this feature can be
implemented. Any pointers or feedback is welcome. I've filed a Bug[2] to
track this issue.

[1]: http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-October/066808.html
[2]: http://llvm.org/bugs/show_bug.cgi?id=17769

-- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted
by The Linux Foundation



-------------- next part --------------
A non-text attachment was scrubbed...
Name: ldr_pseudo.s
Type: application/octet-stream
Size: 4328 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20131101/f3dd6cc3/attachment.obj>

Hi David,

In these examples, I don’t see the directive that indicates where the assembler
should place the constant pool?

-Jim

On Nov 1, 2013, at 11:16 AM, David Peixotto <dpeixott at codeaurora.org>
wrote:
> In an earlier email[1] I proposed adding support for the ldr
> pseud-instruction to the ARM integrated assembler. After some discussion
the
> overall consensus seemed to be that it was worth adding. One concern was
> that we needed to have adequate testing. I promised to provide more details
> on what the behavior should be and provide some tests before starting the
> implementation. The FileCheck-ified tests are attached to this email. They
> currently pass when using gcc to assemble the test.
> 
> Here are the specific behaviors that are checked in the test:
> 
> 1. Check that constants that fit into imm12 are converted to mov
>    ldr r0, =0x3
> 2. Check that large constants are converted to ldr from constant pool
>    ldr r0, =0x103
> 3. Duplicate constants should be merged to the same constant pool location
>    ldr r0, =0x103
>    ...
>    ldr r0, =0x103
> 4. A section defined in multiple pieces is merged and uses a single
constant
> pool
>    For example,
> 
>    .section e, "ax", %progbits
>      f6:
>        ldr r0, =0x103
>    .section f, "ax", %progbits
>      f7: add r0, r0, r0
>    .section e, "ax", %progbits
>      f8:
>        add r0, r0, r0
>        ldr r0, =0x105
> 
>    should only produce one constant pool for section e.
> 
> 5. Check that symbols can be loaded using ldr pseudo
>  ldr r0, =foo
> 
> 6. Check that ldr pseudo can be used with expressions
>  ldr r0, =0x101+6
>  ldr r0, =bar+4
> 
> 7. Check that it is used correctly in a macro
>    .macro useit_in_a_macro
>      ldr r0, =0x101
>      ldr r0, =bar
>    .endm
>    .section k, "ax", %progbits
>    f14:
>      useit_in_a_macro
> 
> 8. Check that an error is issued when the constant pool would be placed too
> far away
>  ldr r0, =0x101
>  @... lots of instructions
>  Error: invalid literal constant: pool needs to be closer
> 
> I have not yet looked into the code to see where/how this feature can be
> implemented. Any pointers or feedback is welcome. I've filed a Bug[2]
to
> track this issue.
> 
> [1]: http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-October/066808.html
> [2]: http://llvm.org/bugs/show_bug.cgi?id=17769
> 
> -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted
> by The Linux Foundation
> 
> 
> 
> <ldr_pseudo.s>_______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

Hi David,
> 8. Check that an error is issued when the constant pool would be placed too
> far away
I'd say this one is actually the most involved constraint but there
don't actually seem to be any tests in the attached file for it.

And I believe the directive Jim's referring to is ".ltorg".
It's
presumably going to have some interesting quirks of its own.

Cheers.

Tim.

Hi Jim,
 
> In these examples, I don't see the directive that indicates where the
> assembler should place the constant pool?
> 
As Tim mentioned, the directive in question is .ltorg. I had planned to
implement that in a separate step once the initial support for ldr-pseudo
was complete. The .ltorg directive is only required when the constant pool
would be placed too far away for the offset to be encoded in the pc-relative
load. It gives the programmer an escape, but is not required for an initial
implementation.

I was thinking that without the .ltorg directive the constant pool would go
at the end of the section.

Hi Tim,
> > 8. Check that an error is issued when the constant pool would be
> > placed too far away
> 
> I'd say this one is actually the most involved constraint but there
don't
> actually seem to be any tests in the attached file for it.
I put the test in a separate file and forgot to attach it earlier. I've
attached it to this email. I can't say how difficult this will be to
implement, but Jim also indicated it could be tricky so I believe you both
:). I only have a simple test for this now, but I'd welcome suggestions on
improving it or any hits on the implementation.

> And I believe the directive Jim's referring to is ".ltorg".
It's
> presumably going to have some interesting quirks of its own.
Yes, I replied to Jim saying that I was planning to implement .ltorg in a
separate step.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: ldr_pseudo_errors.s
Type: application/octet-stream
Size: 17617 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20131101/2215fd20/attachment.obj>

> > I was thinking that without the .ltorg directive the constant pool
> > would go at the end of the section.
> >
> So where does the assembler place the constant pool(s) if that directive
> isn't present? I was under the impression it was always required.
>From my understanding it is not required. I see that GCC will place it at
the end of the section. I don't know if it will ever place it anywhere
besides the end of the section when there is no .ltorg directive.

Here is the relevant section from the gcc docs
(https://sourceware.org/binutils/docs/as/ARM-Directives.html):

"""
.ltorg
This directive causes the current contents of the literal pool to be dumped
into the current section (which is assumed to be the .text section) at the
current location (aligned to a word boundary). GAS maintains a separate
literal pool for each section and each sub-section. The .ltorg directive
will only affect the literal pool of the current section and sub-section. At
the end of assembly all remaining, un-empty literal pools will automatically
be dumped.
"""

On Nov 1, 2013, at 12:06 PM, David Peixotto <dpeixott at codeaurora.org>
wrote:
>>> I was thinking that without the .ltorg directive the constant pool
>>> would go at the end of the section.
>>> 
>> So where does the assembler place the constant pool(s) if that
directive
>> isn't present? I was under the impression it was always required.
> 
> From my understanding it is not required. I see that GCC will place it at
> the end of the section. I don't know if it will ever place it anywhere
> besides the end of the section when there is no .ltorg directive.
> 
> Here is the relevant section from the gcc docs
> (https://sourceware.org/binutils/docs/as/ARM-Directives.html):
> 
> """
> .ltorg
> This directive causes the current contents of the literal pool to be dumped
> into the current section (which is assumed to be the .text section) at the
> current location (aligned to a word boundary). GAS maintains a separate
> literal pool for each section and each sub-section. The .ltorg directive
> will only affect the literal pool of the current section and sub-section.
At
> the end of assembly all remaining, un-empty literal pools will
automatically
> be dumped.
> """
> 
What does ARM’s documentation say?

I have attached an initial patch that implements the ldr pseudo. It still
needs some clean up and more tests, but I would like some feedback on the
approach I used and if there are any objections to implementing it this way.
Here is my approach:

Add a finishParse() callback to the target asm parser
  This callback is invoked when the parse has finished
  successfully. It will be used to write out constant pools
  for the ldr pseudo.

LDR pseudo implementation
  Keep a map from Section => ConstantPool
  When parsing ldr r0, =val
    parse val as an MCExpr
    get ConstantPool for current Section
    remember val in ConstantPool (at next available Offset)
    add operand to ldr that is MCExpr of (ConstantPool.Label + Offset)

  On finishParse() callback
    Write out all non-empty constant pool vals to the end of their sections

Missing features
  1. Does not convert load of small constants to mov
     (e.g. ldr r0, =0x1 => mov r0, 0x1)
  2. Does reuse constant pool entries for same constant

It currently passes the tests I shared earlier (including the error message)
except that ldr is not yet converted to mov when possible. Please let me
know if you have any issues with this method of implementation.

Thanks,
-David

-- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted
by The Linux Foundation
> -----Original Message-----
> From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at
cs.uiuc.edu] On
> Behalf Of David Peixotto
> Sent: Friday, November 01, 2013 11:16 AM
> To: llvmdev at cs.uiuc.edu
> Subject: [LLVMdev] Implementing the ldr pseudo instruction in ARM
> integrated assembler
> 
> In an earlier email[1] I proposed adding support for the ldr pseud-
> instruction to the ARM integrated assembler. After some discussion the
> overall consensus seemed to be that it was worth adding. One concern was
> that we needed to have adequate testing. I promised to provide more
> details on what the behavior should be and provide some tests before
> starting the implementation. The FileCheck-ified tests are attached to
> this email. They currently pass when using gcc to assemble the test.
> 
> Here are the specific behaviors that are checked in the test:
> 
> 1. Check that constants that fit into imm12 are converted to mov
>     ldr r0, =0x3
> 2. Check that large constants are converted to ldr from constant pool
>     ldr r0, =0x103
> 3. Duplicate constants should be merged to the same constant pool location
>     ldr r0, =0x103
>     ...
>     ldr r0, =0x103
> 4. A section defined in multiple pieces is merged and uses a single
> constant pool
>     For example,
> 
>     .section e, "ax", %progbits
>       f6:
>         ldr r0, =0x103
>     .section f, "ax", %progbits
>       f7: add r0, r0, r0
>     .section e, "ax", %progbits
>       f8:
>         add r0, r0, r0
>         ldr r0, =0x105
> 
>     should only produce one constant pool for section e.
> 
> 5. Check that symbols can be loaded using ldr pseudo
>   ldr r0, =foo
> 
> 6. Check that ldr pseudo can be used with expressions
>   ldr r0, =0x101+6
>   ldr r0, =bar+4
> 
> 7. Check that it is used correctly in a macro
>     .macro useit_in_a_macro
>       ldr r0, =0x101
>       ldr r0, =bar
>     .endm
>     .section k, "ax", %progbits
>     f14:
>       useit_in_a_macro
> 
> 8. Check that an error is issued when the constant pool would be placed
> too far away
>   ldr r0, =0x101
>   @... lots of instructions
>   Error: invalid literal constant: pool needs to be closer
> 
> I have not yet looked into the code to see where/how this feature can be
> implemented. Any pointers or feedback is welcome. I've filed a Bug[2]
to
> track this issue.
> 
> [1]: http://lists.cs.uiuc.edu/pipermail/llvmdev/2013-October/066808.html
> [2]: http://llvm.org/bugs/show_bug.cgi?id=17769
> 
> -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-PR17769-Initial-implementation-of-ldr-pseudo.patch
Type: application/octet-stream
Size: 11398 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20131111/b0ffc376/attachment.obj>

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