This is an archive of the discontinued LLVM Phabricator instance.

[RISCV 10/10] Add common fixups and relocations
ClosedPublic

Authored by asb on Aug 16 2016, 8:47 AM.

Details

Summary

%lo(), %hi(), and %pcrel_hi() are supported and test cases have been added to ensure the appropriate fixups and relocations are generated. I've added an instruction format field which is used in RISCVMCCodeEmitter to, for instance, tell whether it should emit a lo12_i fixup or a lo12_s fixup (RISC-V has two 12-bit immediate encodings depending on the instruction type).

Additionally, fixups and relocations in jal and branch instructions are supported.

I actually tried a couple ways of handling this and decided that introspecting the instruction format in RISCVMCCodeEmitter ended up cleanest. I'd be happy to hear any other suggestions though.

This completes the first batch of patches.

Diff Detail

Repository
rL LLVM

Event Timeline

asb updated this revision to Diff 68194.Aug 16 2016, 8:47 AM
asb retitled this revision from to [RISCV 10/10] Add fixups and relocations necessary to support %hi(), %lo(), %pcrel_hi().
asb updated this object.
asb added reviewers: theraven, jyknight.
asb added a subscriber: llvm-commits.
emaste added a subscriber: emaste.Aug 16 2016, 2:43 PM
asb updated this revision to Diff 69358.Aug 26 2016, 5:34 AM

Refresh patch to reflect changes made in earlier patches in the series.

asb updated this revision to Diff 74028.Oct 8 2016, 6:28 AM

Refresh patch to reflect changes earlier in the series.

asb updated this revision to Diff 74381.Oct 12 2016, 7:56 AM

Update test style as suggested by @jyknight in D23564

Razer6 added a subscriber: Razer6.Feb 1 2017, 5:11 AM

@theraven @jyknight What's the state of this change?

theraven edited edge metadata.Feb 1 2017, 6:52 AM

@theraven @jyknight What's the state of this change?

The patch looks fine to me, but I thought it was waiting on the RISC-V ABI document discussing relocation in more detail than 'go see what binutils does'.

asb added a comment.Feb 2 2017, 2:28 AM

@theraven @jyknight What's the state of this change?

There has been progress on documenting aspects of the RISC-V ABI, though I decided to hold off on pushing for finalising the second half of my initial patchset due to concerns some parts of it may change with the addition of codegen support. I've lost more time than I expected to due to travel, other development responsibilities, and trying to secure further funding so I'm a little behind where I expected to be in terms of the codegen patches but I will put them up for review either over the weekend or next week, and also refresh these patches.

asb updated this revision to Diff 88326.Feb 14 2017, 12:54 AM
asb retitled this revision from [RISCV 10/10] Add fixups and relocations necessary to support %hi(), %lo(), %pcrel_hi() to [RISCV 10/10] Add common fixups and relocations.
asb edited the summary of this revision. (Show Details)

This refresh adds support for relocations and fixups on branch and jal instructions. This is sufficient for compiling simple programs.

The intent is this patch is now ready for merging.

asb updated this revision to Diff 88329.Feb 14 2017, 1:03 AM

Here's the same patch as before with full context, apologies for the noise.

asb updated this revision to Diff 110344.Aug 9 2017, 3:54 AM

This updated version of the patch reflects upstream changes to arguments of applyFixup, and takes advantage of my work in rL299529 to report errors for out-of-range or insufficiently aligned fixups.

majnemer added inline comments.
lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
557–561 ↗(On Diff #110344)

This would be more clear as:

// It's a simple symbol reference with no addend.
if (isa<MCSymbolRefExpr>(Expr))
  return true;
560 ↗(On Diff #110344)

Is it your intention to return true with Kind equal to RISCVMCExpr::VK_RISCV_Invalid?

582 ↗(On Diff #110344)

Didn't your just isa<>? I think this can be cast<>.

asb updated this revision to Diff 111501.Aug 17 2017, 5:09 AM
asb marked 2 inline comments as done.

Updated to address @majnemer's review comments (thanks!).

lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
560 ↗(On Diff #110344)

It's not, good catch. Initialising Kind to RISCVMCExpr::VK_RISCV_None seems to be the correct thing to do.

582 ↗(On Diff #110344)

I think this is the first time we check for MCConstantExpr. Unless I'm misunderstanding you, I don't think this has to be a dyn_cast

dylanmckay added inline comments.Aug 21 2017, 8:18 AM
lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
12 ↗(On Diff #111501)

This is not in alphabetical order

455 ↗(On Diff #111501)

redundant break

lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp
42 ↗(On Diff #111501)

Do you need this cast here? You save a similar switch in RISCVAsmBackend without one

lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
170 ↗(On Diff #111501)

Nitpick: The double nesting and duplicating of the unreachable isn't that intuitive - we might be able to simplify the control flow by initialising FixupKind to a sentinel value, dropping both else { unreachable } blocks, and then just placing a if (FixupKind == sentinel) { unreachable }

asb updated this revision to Diff 112163.Aug 22 2017, 6:15 AM
asb marked 3 inline comments as done.

Address review comments from @dylanmckay (thanks!).

lib/Target/RISCV/MCTargetDesc/RISCVELFObjectWriter.cpp
42 ↗(On Diff #111501)

The cast is needed to avoid a warning "case value not in enumerated type 'llvm::MCFixupKind'".

dylanmckay added inline comments.Aug 22 2017, 10:23 PM
lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp
169 ↗(On Diff #112163)

No else after return

183 ↗(On Diff #112163)

Can we factor any code out from both isSImm13Lsb0 and the other signed immediated lsb functions? It seems like the only difference in these functions are the sizes of the immediates

If the same could be done for the UImm functions, that'd be good, although a quick glance shows that there is some differing logic based on the variant

186 ↗(On Diff #112163)

No else after return

197 ↗(On Diff #112163)

No else after return

210 ↗(On Diff #112163)

No else after return

273 ↗(On Diff #112163)

Is there a reason why this is only executed for RISCVMCExpr objects? AFAIK, evaluateAsConstant is also supported by MCExpr, and so I'm not sure if you're intentionally not evaluating those expressions here.

477 ↗(On Diff #112163)

Should this be getIdentifier()?

It seems strange that you'd verify that the token is an identifier in the above if, but then read a string instead.

lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
36 ↗(On Diff #112163)

I don't think we want a full stop here because it might muck up the output of llc --stats, or at least make it look funny

asb marked 4 inline comments as done.Aug 23 2017, 4:19 AM

Thanks for the comments Dylan. Playing around with this, I've noticed that the MC assembler is too accepting of some inputs (e.g. accepting addi a1, a2, %hi(foo) where gas would reject it). I'm fixing this and expanding tests now, which will change many of the isFoo functions anyway. I'll watch for the else after return thing. I personally don't find that guideline very compelling for else if cases where removing the else fails to reduce indentation, but I shall of course conform with LLVM norms :)

The last patch update didn't have full context included - sorry for that.

dylanmckay edited edge metadata.Aug 23 2017, 4:51 AM

No problem, I remember it being quite hard to merge my own backend in-tree due to reviewers being scarce, thought I might as well help out :)

I personally don't find that guideline very compelling for else if in cases where removing the else fails to reduce indentation

I can definitely understand that, at first I thought that the no else after return rule only applied to else, but I checked before this review and was surprised as well.

The last patch update didn't have full context included - sorry for that.

No problem - that reminds me that I just made the same mistake on a patchset I just created!

asb updated this revision to Diff 112352.Aug 23 2017, 6:44 AM
asb marked 3 inline comments as done.

Previous versions of this patch were too accepting of invalid inputs, e.g. accepting addi a0, a0, %hi(1). Previously there was logic that would try to evaluate a RISCVMCExpr as a constant in createImm (i.e. as early as possible). This throws away information about any operand modifier that was parsed. Therefore, move to evaluating the constant in in the isFoo() predicate arguments, which also checks whether any operand modifiers were allowed. rv32i-invalid.s has been expanded to systematically test these issues. As suggested by Dylan, I've also added a helper function to factor out the commonality between the isSImmNNLsb0 functions.

asb updated this revision to Diff 112355.Aug 23 2017, 6:48 AM

Just spotted a minor issue. RISCVMCExpr::getVariantKindForName would be better returning VK_RISCV_Invalid if the name is unrecognised rather htna VK_RISCV_None. Updated diff fixes this.

asb marked an inline comment as done.Sep 5 2017, 12:19 PM

Ping?

mgrang added a subscriber: mgrang.Sep 6 2017, 5:10 PM
mgrang added inline comments.
lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
68 ↗(On Diff #112355)

Extra newline?

181 ↗(On Diff #112355)

Do we really need to use another variable Idx? Can't we simply use i?

lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp
68 ↗(On Diff #112355)

Default should be the first case in a switch.

96 ↗(On Diff #112355)

Ditto.

asb updated this revision to Diff 114139.Sep 7 2017, 3:22 AM
asb marked 4 inline comments as done.

Update to address review comments (thanks @mgrang!).

psnobl added a subscriber: psnobl.Sep 8 2017, 11:36 AM
asb updated this revision to Diff 115574.Sep 17 2017, 7:52 AM

Refresh patch based on updates to others in series.

@dylanmckay, are you happy with this now?

dylanmckay accepted this revision.Sep 27 2017, 3:00 PM
This revision is now accepted and ready to land.Sep 27 2017, 3:00 PM
This revision was automatically updated to reflect the committed changes.
sabuasal added inline comments.
llvm/trunk/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
148

I think this is problematic, fixup_riscv_lo12_i is only used for S instruction format in RISCV. I think this should be
if (MIFrom == RISCVII::InstFormatI) {

FixUpKind = RISCV::fixup_riscv_lo12_i;

} else if (RISCVII::InstFormatS) {
FixUpKind = RISCV::fixup_riscv_lo12_s;
} else {

llvm_unreacable("unexpected Instructio type for relocation  FixUpKind = RISCV::fixup_riscv_lo12_i");

}

Other wise you'll mistakenly end up generating instructions instructions with wrong fixup kinds!

sabuasal added inline comments.Feb 15 2018, 7:03 PM
llvm/trunk/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
148

correction: *fixup_riscv_lo12_s* is only used for S instruction format in RISCV.

asb added inline comments.Feb 22 2018, 5:28 AM
llvm/trunk/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp
148

You're right this code path isn't written very defensively, and it may start to fail if VK_RISCV_LO is used with different instruction formats. I've addressed this in rL325775. Thanks for spotting this.