Adds support for LA expansion in PIC mode.
Patch By: Srdjan Obucina
Details
Details
Diff Detail
Diff Detail
- Build Status
Buildable 3719 Build 3719: arc lint + arc unit
Event Timeline
Comment Actions
The code change looks right to me but I have some comments about the test case.
Rather than make a new test file, could you extend test/MC/Mips/macro-la.s? This is mostly to keep our test directories tidy but it will also allow us to test that the other cases (e.g. 'la $5, 1') are unaffected and we'll have better coverage of the variety of expressions (e.g. 'la $5, symbol+8($6)', 'la $6, symbol+8($6)', and 'la $5, 1f').
Comment Actions
We couldn't extend test/MC/Mips/macro-la.s because some tests in it failed in pic mode. For example, 'la $5, symbol+8' gives: 'LLVM ERROR: unsupported reloc value'. Also, istruction la in the case 'la $5, 1f' does not expand properly because symbol 1 is defined at the end of the test file, so condition 'Sym->isInSection()' fails because of one pass structure of assembler.
| lib/Target/Mips/AsmParser/MipsAsmParser.cpp | ||
|---|---|---|
| 2801 | The problem here is that you do not know whether a symbol is going to have global or local linkage until the end of the module as the .global directive may appear later. In order to do this you will need to track which symbols are referred to during macro expansion and then at the end of the file check that all the symbols still have the same linkage. If any have changed then raise an error. The restriction you will end up with in LLVM is that a global symbol must be globalized before the first reference to it. The test cases are: global.text local.data global.data local (but an error for LLVM).text global (but an error for LLVM).data If the cases which work are sufficient for the codebases you need to support then this is relatively easy to implement. If you need the last two cases to work then there will be a significant amount of framework required as I understand. | |
Comment Actions
For example, 'la $5, symbol+8' gives: 'LLVM ERROR: unsupported reloc value'.
Whatever caused this seems to have been fixed. It emits this for me:
lw $6, %got(symbol)($gp) # encoding: [A,A,0x86,0x8f]
| lib/Target/Mips/AsmParser/MipsAsmParser.cpp | ||
|---|---|---|
| 2801 |
I should have noticed that given that we've recently had the same problem with jal.
I like this solution. There's some valid inputs to GAS that won't be supported but it only takes a trivial source change (inserting a '.global bar') to work around it. I'd limit the 'reference' part of your last sentence to just references involved in this kind of linkage-specific expansion since things like: .data foo: .word bar bar: .word 1 aren't a problem. We only need one of the two error cases to be an error don't we? I think we could assume local and error out if it turned out to be global, or assume global and error out if it turned out to be local.
That's my understanding too. We don't have a way to see the whole input before we have to make a decision on how to expand. | |
| test/MC/Mips/la.pic.s | ||
| 15 ↗ | (On Diff #47453) | This instruction is incomplete |
| lib/Target/Mips/AsmParser/MipsAsmParser.cpp | ||
|---|---|---|
| 2801 |
No I think both ways round will happen. All symbols will need to start off presumed to be global to match gas and for the case where they are not defined in the current module at all. If they are defined without .global before being referenced then they are local but could become global later. If they are not declared before being referenced they are global but may become local if defined later. Only if we assumed local could we eliminate one of the error cases but that would lead to all externally defined globals needing an explicit .global in each file that references them... which is contrary to gas behaviour. Not sure if that description is too technically dense. I'll try again if it is impenetrable. | |
| lib/Target/Mips/AsmParser/MipsAsmParser.cpp | ||
|---|---|---|
| 2801 | I see what I've missed. The assumptions needed to make either of the latter two cases 'just work' directly contradict the assumptions needed to make the first three cases work. The overall assumptions when processing the 'la $4, bar' are:
| |
Comment Actions
Update to trunk, account in tests for difference between emitting assembly and objects.
Comment Actions
Just removing a 3 year old patch from my queue. Feel free to re-add me if this is still needed and you want me to take another look
The problem here is that you do not know whether a symbol is going to have global or local linkage until the end of the module as the .global directive may appear later. In order to do this you will need to track which symbols are referred to during macro expansion and then at the end of the file check that all the symbols still have the same linkage. If any have changed then raise an error. The restriction you will end up with in LLVM is that a global symbol must be globalized before the first reference to it.
The test cases are:
global
.text
foo:
la $4, bar
local
.data
bar:
.word 1
.text
foo:
la $4, bar
global
.data
.global bar
bar:
.word 1
.text
foo:
la $4, bar
local (but an error for LLVM)
.text
foo:
la $4, bar
.data
bar:
.word 1
global (but an error for LLVM)
.data
bar:
.word 1
.text
foo:
la $4, bar
.global bar
If the cases which work are sufficient for the codebases you need to support then this is relatively easy to implement. If you need the last two cases to work then there will be a significant amount of framework required as I understand.