Page MenuHomePhabricator

[RISCV][MC] Fixed error: could not find corresponding %pcrel_hi
AbandonedPublic

Authored by apazos on Feb 27 2019, 8:50 PM.

Details

Reviewers
eli.friedman
Summary

When parsing instructions, we need to provide the subtarget to detect when the subtarget changes due to options as they are parsed.
This affects the fragment the label will be placed in.

Diff Detail

Event Timeline

apazos created this revision.Feb 27 2019, 8:50 PM

Lewis, is this what you had in mind, I only changed RISC-V parser path.

Lewis, is this what you had in mind, I only changed RISC-V parser path.

Yes, something very similar, thank you. I just added an additional EmitLabel, with a default implementation which just called the original EmitLabel, and override it for MCELFStreamer. If this patch compiles for all targets though I don't see any problem with it.

apazos added a comment.Mar 1 2019, 1:03 PM

Hi Lewis, yes it is possible to reduce the code changes. I will push an update.
The concern I have is that for pseudo instructions it was enough to change RISCVAsmParser.cpp to fix the problem. But for the expanded instructions I have to modify AsmParser.cpp.
So maybe instead of adding a new EmitLabel we change the current api.

apazos updated this revision to Diff 188962.Mar 1 2019, 1:08 PM

Reduced code changes, added test case

apazos edited reviewers, added: eli.friedman; removed: lewis-revill.Mar 1 2019, 1:10 PM
apazos added a subscriber: llvm-commits.
apazos retitled this revision from [RISCV] (WIP) Fixed error: could not find corresponding %pcrel_hi to Fixed error: could not find corresponding %pcrel_hi.Mar 1 2019, 1:24 PM
apazos edited the summary of this revision. (Show Details)
apazos updated this revision to Diff 188965.Mar 1 2019, 1:27 PM

Updated triple in test case

I don't understand why it necessary to ensure the label is attached to the "correct" fragment; it's still pointing to the same address either way. Could you try a little harder in RISCVMCExpr::getPCRelHiFixup() to find the symbol? (Given an MCFragment, you can call getPrevNode() to get the previous MCFragment.)

I don't understand why it necessary to ensure the label is attached to the "correct" fragment; it's still pointing to the same address either way. Could you try a little harder in RISCVMCExpr::getPCRelHiFixup() to find the symbol? (Given an MCFragment, you can call getPrevNode() to get the previous MCFragment.)

I guess the difficulty there is that you wouldn't know which fragment the symbol offset (AUIPCSymbol->getOffset()) relates to. So you might end up finding a symbol with a matching offset in the current fragment even though you should be looking in the previous fragment, or vice-versa.

Could you re-title the revision to start with [RISCV] and preferably [MC]? As it is, it may not be seen by all the relevant people.

apazos retitled this revision from Fixed error: could not find corresponding %pcrel_hi to [RISCV][MC] Fixed error: could not find corresponding %pcrel_hi.Mar 4 2019, 10:15 AM
apazos added a comment.Mar 4 2019, 1:42 PM

Hi Eli,

The pair of instructions constraint is that the instruction (or instructions) with the LO12 relocation label points to a valid HI20 relocation which in turn points to the symbol:

At label: R_RISCV_PCREL_HI20 relocation entry ⟶ symbol
R_RISCV_PCREL_LO12_I relocation entry ⟶ label

See https://github.com/riscv/riscv-elf-psabi-doc/blob/master/riscv-elf.md#relocations

Let us know if you have a suggestion on how to do this matching some other way.

I guess the difficulty there is that you wouldn't know which fragment the symbol offset (AUIPCSymbol->getOffset()) relates to.

Either the label is in the same fragment as the fixup, so the current code works, or the label points to the end of a fragment and the fixup is at offset zero in the next fragment. Otherwise, they can't match because there's data in between the label and the fixup. (I guess it's a little more complicated if you consider the possibility of zero-size fragments, but that doesn't make it a lot more complicated.)

I'm concerned the approach in current patch can't be generalized to handle all cases, for example:

.option push
.option norelax
nop
2:
.option pop
auipc   a1, %pcrel_hi(another_symbol)
addi    a1, a1, %pcrel_lo(2b)
include/llvm/MC/MCStreamer.h
443

If we're going to do this, we should give this a better name... maybe something like "EmitLabelInNextFragment" or something like that, to indicate that it's actually doing something unusual.

Posted https://reviews.llvm.org/D58943, which implements my proposal. Hopefully it makes sense.

apazos added a comment.Mar 4 2019, 8:32 PM

Hi Eli, this current patch as it is fails for the test case you highlighted. Thanks for clarifying and pushing an alternative solution.

apazos abandoned this revision.Mar 6 2019, 4:53 PM

We are going with the alternative in https://reviews.llvm.org/D58943