David Peixotto
2013-Dec-17 19:33 UTC
[LLVMdev] Implementing the ldr pseudo instruction in ARM integrated assembler
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>
Jim Grosbach
2013-Dec-17 19:41 UTC
[LLVMdev] Implementing the ldr pseudo instruction in ARM integrated assembler
On Dec 17, 2013, at 11:33 AM, David Peixotto <dpeixott at codeaurora.org> wrote:> 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. >Ack! Sorry about that. That would certainly explain some of my confusion. I suspect some (mis)threading of emails got in the way. Thanks again for your patience working through this. Responses to a few specific questions below, and I’ll follow up on the new patch separately.>> Maybe I'm just blind, but where's the code to handle the .ltorg directive? > > It is implemented in patch 0003 in this email.Excellent.> >> +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.A bit of both. I feel strongly that there should be tests that don’t emit to object files, but do agree that there’s value to checking object as well (for the range checking in particular). In particular, the asm streamer tests can probably be platform independent, or at least close. The object file tests obviously cannot. For the relocations, is there something specific about the code generated here that tests those in ways that existing relocation tests don’t cover? I would have expected that part to be totally generic, as the pseudo is just a syntactic convenience after all and not expressing anything that can’t already be written in a .s file without the construct.> >> 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.Will do, and thanks again. -Jim> > <0001-Add-a-finishParse-callback-to-the-targer-asm-parser.patch><0002-Implement-the-ldr-pseudo-opcode-for-ARM-assembly.patch><0003-Implement-the-.ltorg-directive-for-ARM-assembly.patch><squashed.patch>
David Peixotto
2013-Dec-17 20:00 UTC
[LLVMdev] Implementing the ldr pseudo instruction in ARM integrated assembler
> > 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. > > > Ack! Sorry about that. That would certainly explain some of my confusion. > I suspect some (mis)threading of emails got in the way. Thanks again for > your patience working through this.Yes there were a number of patches spread out over a period of time. I am close to getting permission to use phabricator which should help alleviate this problem in the future.> >> 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. > > A bit of both. I feel strongly that there should be tests that don't emit > to object files, but do agree that there's value to checking object as > well (for the range checking in particular). In particular, the asm > streamer tests can probably be platform independent, or at least close. > The object file tests obviously cannot. For the relocations, is there > something specific about the code generated here that tests those in ways > that existing relocation tests don't cover? I would have expected that > part to be totally generic, as the pseudo is just a syntactic convenience > after all and not expressing anything that can't already be written in a > .s file without the construct.I wrote the tests before I did the actual implementation and I thought I would be generating the relocations myself. The implementation turned out to be much simpler than I imagined and I was able to lean on the existing support for generating relocations and fixups. I expect all these relocations would be covered by other tests. I think we still would need separate tests for darwin/linux because the syntax for creating sections (for example) is different. One option is to leave the object test for the errors (out of range fixup) and switch to asm tests for the others.
Possibly Parallel Threads
- [LLVMdev] Implementing the ldr pseudo instruction in ARM integrated assembler
- [LLVMdev] Implementing the ldr pseudo instruction in ARM integrated assembler
- [LLVMdev] Implementing the ldr pseudo instruction in ARM integrated assembler
- [LLVMdev] Implementing the ldr pseudo instruction in ARM integrated assembler
- [LLVMdev] Implementing the ldr pseudo instruction in ARM integrated assembler