Page MenuHomePhabricator

[RISCV] Use Generated Instruction Uncompresser

Authored by lenary on Jul 2 2020, 9:57 AM.



This change allows relaxInstruction to use the auto-generated
instruction uncompression routine, rather than a partial implementation
of the same.

I am yet to find a testcase that this affects.

Diff Detail

Unit TestsFailed

370 msx64 debian > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp

Event Timeline

lenary created this revision.Jul 2 2020, 9:57 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 2 2020, 9:57 AM
lenary planned changes to this revision.Jul 2 2020, 10:13 AM

Oh there's more to do, which may involve changes to llvm/utils/TableGen/RISCVCompressInstEmitter.cpp

lenary updated this revision to Diff 275166.Jul 2 2020, 10:51 AM
  • Enable More Relaxations
luismarques accepted this revision.Jul 6 2020, 9:44 AM



Nit: declare Res at the point of initialization or, probably better yet, just do the if on the result of uncompressInst.

This revision is now accepted and ready to land.Jul 6 2020, 9:44 AM
asb added a comment.Jul 14 2020, 11:04 PM

I'm getting a test failure on rv64-relax-all.s with this patch?

lenary updated this revision to Diff 305793.Nov 17 2020, 7:30 AM
  • Fix up rv64-relax-all.s testcase
lenary updated this revision to Diff 305794.Nov 17 2020, 7:34 AM
  • Address review nit.
lenary updated this revision to Diff 305795.Nov 17 2020, 7:35 AM
  • Rebase
Harbormaster completed remote builds in B79122: Diff 305793.

Issues I still need to investigate:

  • That this doesn't cause a code-size blow-up
  • That this doesn't cause compiler performance regressions due to using uncompressInst in mayNeedRelaxation

However, this change is also blocking relaxing conditional branches, so I need to get to a conclusion on this quickly.

Preliminary data says there's no major difference to code size from this patch, so I'm feeling more confident about it. I still want to do a GCC Torture test run, if possible.

rkruppe removed a subscriber: rkruppe.Nov 19 2020, 12:02 PM
asb added a comment.EditedNov 26 2020, 8:22 AM

I'm afraid this causes compile-time failures with 20030323-1.c, multi-ix.c, and pr53645.c.


+./output_rv64imafdc_lp64_O0/multi-ix.o: in function `.LBB0_1':
+multi-ix.c:(.text+0x60): relocation truncated to fit: R_RISCV_BRANCH against `.LBB0_4'
+clang-11: error: linker command failed with exit code 1 (use -v to see invocation)
jrtc27 added inline comments.Nov 26 2020, 8:42 AM

Only if Inst is a non-indirect branch (which is exactly the set that getRelaxedOpcode previously handled)?

7–9 ↗(On Diff #305795)

This seems like a regression?

17–21 ↗(On Diff #305795)

Why not test with a non-zero immediate like before?

lenary updated this revision to Diff 307911.Nov 26 2020, 12:27 PM

So, looking further into this for the failing test case:

  • The patch as-was relaxed far more instructions than necessary in e.g. the example @asb notes is failing.
  • I went back to using the explicit list of instructions that can be relaxed, from feedback from @jrtc27 - as the list is not likely to grow.
  • This change is now probably pointless, as the uncompresser is *huge* and the list of instructions we should uncompress is all of 4.
lenary abandoned this revision.Nov 26 2020, 12:33 PM