[PATCH] D43157: [RISCV] Properly evaluate VK_RISCV_PCREL_LO

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.
Herald added a subscriber: shiva0217.

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.



================
Comment at: lib/Target/RISCV/MCTargetDesc/RISCVMCExpr.cpp:15
 
+#include "RISCVFixupKinds.h"
 #include "RISCVMCExpr.h"
----------------
Nit: should appear below RISCVMCExpr.h ("main module header" goes first https://llvm.org/docs/CodingStandards.html#include-style).


https://reviews.llvm.org/D43157