[PATCH] D43328: [RISCV] Update MC compression tests

[Sameer AbuAsal via Phabricator via llvm-commits]

sabuasal added inline comments.


================
Comment at: test/MC/RISCV/compress-relocations.s:1-13
+# RUN: llvm-mc -triple riscv32 -mattr=+c -riscv-no-aliases < %s -show-encoding \
+# RUN:     | FileCheck -check-prefix=INSTR -check-prefix=FIXUP %s
+
+# Check prefixes:
+# FIXUP - Check the fixup on the instruction.
+# INSTR - Check the instruction is handled properly by the ASMPrinter
+c.jal foo
----------------
asb wrote:
> sabuasal wrote:
> > asb wrote:
> > > asb wrote:
> > > > You've lost the RELOC checks.
> > > Also if we're going to separate this to a separate file, relocations-compressed.s would be more consistent with naming of other files (such as fixups-compressed.s).
> > I actually didn't forget. This is a bit tricky.
> > 
> > MC for RISCV , as of now, only generates relocations if the target for the branch\jump is unresolved. MC also relaxes branches\jumps to unresolved targets (look at MCAssembler::handleFixup and  MCAssembler::fixupNeedsRelaxation).
> > With relaxation implemented, a compressed branch\jump to an undefined reference will always get relaxed to the expanded form and the relocation generated will be that of the expanded form.
> > 
> > One way this could be circumvented (if we need to) is by implementing shouldForceRelocation, this way a compressed branch to a local (resolved) symbol will also get a relocation.
> I think the correct thing to do is to leave the RELOC checks in this patch, then update the checks in the patch that actually changes the behaviour.
I agree.  Actually I think I'll take your advice on this and on holding off committing this patch till after the main compression patches are reviewed.  I'll update the dependencies accordingly.

I integrated the change we need to the relocation to the relaxation patch (D43055) to make it easier to trace (updating the relocation in relocations.s and adding a func label in fixups-comperessed with checking for not generating a relocation). I will not separate the relocations.s file for now file for now until we figure out the other patches first.


https://reviews.llvm.org/D43328