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

[Jim Grosbach]

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