Page MenuHomePhabricator

[RISCV] Properly evaluate VK_RISCV_PCREL_LO
Needs ReviewPublic

Authored by Bigcheese on Feb 10 2018, 3:16 AM.

Details

Reviewers
asb
ahmedcharles
Summary

VK_RISCV_PCREL_LO has to be handled specially. The MCExpr inside is
actually the location of an auipc instruction with a VK_RISCV_PCREL_HI fixup
pointing to the real target.

Diff Detail

Event Timeline

Bigcheese created this revision.Feb 10 2018, 3:16 AM
asb added a comment.Feb 20 2018, 12:50 AM

Sorry for the delay, I've been on paternity leave. Will review this and D43158 ASAP.

asb added a comment.Mar 14 2018, 10:19 AM

And I have to apologise again that it has taken longer than expected to get back to you on this. I hope my slow feedback this time won't put you off future contributions - going forward, things should be much faster.

I fully agree that the pcrel_lo handling was incorrect and the code you've written seems sensible. Are you convinced that every case in the code that can reasonably exercised by tests is tested? We've now get just two instances of checking this pcrel_lo+pcrel_hi behaviour, which feels a little light.

Nit: The contents of test/MC/RISCV/pcrel-hilo.s should really go in relocations.s or a new relocations-pcrel-hilo.s.

lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp
15

Nit: should appear below RISCVMCExpr.h ("main module header" goes first https://llvm.org/docs/CodingStandards.html#include-style).

PkmX added a subscriber: PkmX.Aug 13 2018, 10:24 PM
PkmX added inline comments.
lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
42

This should also check for lo12_s.

42

Cast to unsigned to silence the warning about mismatching enum type.

lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp
60

Cast to unsigned to silence the warning about mismatching enum type.

test/MC/RISCV/fixups.s
31

Need a test case for S-type.

test/MC/RISCV/pcrel-hilo.s
14

Need a test case for S-type.

PkmX added a comment.Sep 4 2018, 11:23 PM

Ping? This is the last issue blocking lld's RISC-V tests to be rewritten to use llvm-mc and llvm-objdump right now.

This patch also does not apply to trunk now, so it needs to be rebased and modified to handle S-type cases.

PkmX added a comment.Nov 1 2018, 12:51 AM

Ping?

I have a patch available that handles PCREL_LO12_S and is rebased on trunk, if it is okay I will create a separate patch for reviewing.

Ping?

I have a patch available that handles PCREL_LO12_S and is rebased on trunk, if it is okay I will create a separate patch for reviewing.

Sure, that's fine. I won't have any time to look at this until after next week, but feel free to add me as a reviewer.