llvm dev - Nov 2013 - [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 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
> 
> 
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-PR17769-Initial-implementation-of-ldr-pseudo.patch
Type: application/octet-stream
Size: 12883 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20131112/2200e8ac/attachment.obj>

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