Page MenuHomePhabricator

[mips] N64 static relocation model support
ClosedPublic

Authored by sdardis on Aug 18 2016, 3:23 AM.

Details

Summary

This patch makes one change to GOT handling and two changes to N64's
relocation model handling. Furthermore, the jumptable encodings have
been corrected for static N64.

Big GOT handling is now done via a new SDNode MipsGotHi - this node is
unconditionally lowered to an lui instruction.

The first change to N64's relocation handling is the lifting of the
restriction that N64 always uses PIC. Now it possible to target static
environments.

The second change adds support for 64 bit symbols and enables them by
default. Previously N64 had patterns for sym32 mode only. In this mode all
symbols are assumed to have 32 bit addresses. sym32 mode support
is selectable with attribute 'sym32'. A follow on patch for clang will
add the necessary frontend parameter.

Diff Detail

Repository
rL LLVM

Event Timeline

sdardis updated this revision to Diff 68504.Aug 18 2016, 3:23 AM
sdardis retitled this revision from to [mips] N64 static relocation model support.
sdardis updated this object.
sdardis added reviewers: dsanders, vkalintiris.
sdardis added subscribers: theraven, seanbruno, llvm-commits.

It appears clang already defaults to abicalls mode for mips / mips64. I'll correct the summary.

sdardis updated this object.Aug 18 2016, 3:30 AM

Hmmm ... FreeBSD builds for MIPS64 do *not* like this very much.

I've posted the error files here:
https://people.freebsd.org/~sbruno/base64_5570de.tgz

Assertion failed: (isImm() && "Wrong MachineOperand accessor"), function getImm, file /home/sbruno/clang/llvm/include/llvm/CodeGen/MachineOperand.h, line 420.
clang-3.9: error: unable to execute command: Abort trap (core dumped)
clang-3.9: error: clang frontend command failed due to signal (use -v to see invocation)
clang version 4.0.0 (trunk 279404)
Target: mips64-unknown-freebsd12.0
Thread model: posix
InstalledDir: /home/sbruno/clang/build/bin
clang-3.9: note: diagnostic msg: PLEASE submit a bug report to http://llvm.org/bugs/ and include the crash backtrace, preprocessed source, and associated run script.
clang-3.9: note: diagnostic msg:


PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT:
Preprocessed source(s) and associated run script(s) are located at:
clang-3.9: note: diagnostic msg: /tmp/base64-5570de.c
clang-3.9: note: diagnostic msg: /tmp/base64-5570de.sh
clang-3.9: note: diagnostic msg:

Thanks for the report. I suspected that there may still be some assumptions of N64 => position independent. I'll try to get back to this patch but I'm a bit swamped at the moment.

emaste added a subscriber: emaste.Aug 22 2016, 2:12 PM

Looking at this, it's MipsSEDAGToDAGISel::replaceUsesWithZeroReg making an unguarded assumption that an instruction such as daddiu $X, $zero always has an immediate for its third operand (natural counting). The test case supplied has a global address instead of an immediate, triggering the failure.

sdardis updated this revision to Diff 69092.Aug 24 2016, 3:02 AM

Harden replaceUsesWithZeroReg to avoid assumptions.

Note, that new version doesn't generate the same relocations as Gcc. I'm currently tracking that issue down.

sdardis updated this revision to Diff 69104.Aug 24 2016, 6:16 AM

Mark objects that use -mno-abicalls as non-pic.

seanbruno accepted this revision.EditedAug 26 2016, 1:31 PM
seanbruno added a reviewer: seanbruno.

Not sure if you want to change this at all. This is the first time I've been able to get a FreeBSD MALTA64 kernel to boot in QEMU. :-)

% qemu-system-mips64 -m 512M -M malta -kernel /var/tmp/mips.mips64/home/sbruno/mips64-clang/sys/MALTA64/kernel -hda ./mips64_clang.img -nographic
WARNING: Image format was not specified for './mips64_clang.img' and probing guessed raw.
         Automatically detecting the format is dangerous for raw images, write operations on block 0 will be restricted.
         Specify the 'raw' format explicitly to remove the restrictions.
entry: platform_start()
cmd line: /var/tmp/mips.mips64/home/sbruno/mips64-clang/sys/MALTA64/kernel  
envp:
        memsize = 268435456
        ememsize = 536870912
        modetty0 = 38400n8r
memsize = 268435456 (0x10000000)
ememsize = 536870912
Cache info:
  icache is virtual
  picache_stride    = 8192
  picache_loopcount = 4
  pdcache_stride    = 4096
  pdcache_loopcount = 8
cpu0: MIPS Technologies processor v160.130
  MMU: Standard TLB, 48 entries (4K 16K 64K 256K 1M 16M 64M 256M pg sizes)
  L1 i-cache: 4 ways of 256 sets, 32 bytes per line
  L1 d-cache: 4 ways of 256 sets, 32 bytes per line
  L2 cache: disabled
  Config1=0xdea3519b<PerfCount,WatchRegs,EJTAG,FPU>
  Config2=0x80000000
Physical memory chunk(s):
0x71e000 - 0xfffffff, 260972544 bytes (63714 pages)
0x90000000 - 0x9fffffff, 268435456 bytes (65536 pages)
Maxmem is 0xa0000000
KDB: debugger backends: ddb
KDB: current backend: ddb
Copyright (c) 1992-2016 The FreeBSD Project.
Copyright (c) 1979, 1980, 1983, 1986, 1988, 1989, 1991, 1992, 1993, 1994
        The Regents of the University of California. All rights reserved.
FreeBSD is a registered trademark of The FreeBSD Foundation.
FreeBSD 12.0-CURRENT #5 r304849:304856: Fri Aug 26 20:21:52 UTC 2016
    sbruno@tasty.ysv.freebsd.org:/var/tmp/mips.mips64/home/sbruno/mips64-clang/sys/MALTA64 mips
clang version 4.0.0 (trunk 279820)
Preloaded elf kernel "kernel" at 0xffffffff80711d70.
real memory  = 536870912 (524288K bytes)
Physical memory chunk(s):
0x0083b000 - 0x0fffffff, 259805184 bytes (63429 pages)
0x90000000 - 0x9f2e0fff, 254676992 bytes (62177 pages)
avail memory = 511922176 (488MB)
Trap cause = 4 (address error (load or I-fetch) - kernel mode)
[ thread pid 0 tid 100000 ]
Stopped at      vm_object_allocate+0x68:        ld      at,-3088(at)
db> bt
Tracing pid 0 tid 100000 td 0xffffffff80719fd0
db_trace_thread+80 (?,?,?,?) ra ffffffff80162af0 sp 9800000000737130 sz 416
ffffffff80162968+188 (?,?,?,?) ra ffffffff80162488 sp 98000000007372d0 sz 64
ffffffff80162100+388 (?,?,?,?) ra ffffffff801620c4 sp 9800000000737310 sz 224
db_command_loop+dc (?,?,?,?) ra ffffffff80167804 sp 98000000007373f0 sz 48
ffffffff80167648+1bc (?,?,?,?) ra ffffffff803713ec sp 9800000000737420 sz 816
kdb_trap+1cc (?,?,?,?) ra ffffffff8059635c sp 9800000000737750 sz 80
ffffffff8059621c+140 (?,?,?,?) ra 0 sp 98000000007377a0 sz 0
pid 0
db>
This revision is now accepted and ready to land.Aug 26 2016, 1:31 PM

Are you folks ready for this to land? It sure is a huge improvement from my perspective.

seanbruno requested changes to this revision.Sep 15 2016, 7:56 AM
seanbruno edited edge metadata.

I seem to get one reject from this patch:

./test/CodeGen/Mips/fcopysign-f32-f64.ll.rej

@@ -38,8 +38,8 @@
 
 ; 64-DAG: mfc1    $[[MFC:[0-9]+]], $f13
 ; 64-DAG: srl     $[[SRL:[0-9]+]], $[[MFC:[0-9]+]], 31
-; 64:     dsll    $[[DSLL:[0-9]+]], $[[SRL]], 63
 ; 64-DAG: daddiu  $[[R1:[0-9]+]], $zero, 1
+; 64:     dsll    $[[DSLL:[0-9]+]], $[[SRL]], 63
 ; 64-DAG: dsll    $[[R2:[0-9]+]], $[[R1]], 63
 ; 64-DAG: daddiu  $[[R3:[0-9]+]], $[[R2]], -1
 ; 64-DAG: dmfc1   $[[R0:[0-9]+]], ${{.*}}
This revision now requires changes to proceed.Sep 15 2016, 7:56 AM

The code that is changing seems to apply fine, but three of the test files now fail to apply cleanly:

./test/CodeGen/Mips/fcopysign-f32-f64.ll.rej
./test/CodeGen/Mips/llvm-ir/call.ll.rej
./test/CodeGen/Mips/tailcall/tailcall.ll.rej

sdardis updated this revision to Diff 76534.Nov 1 2016, 3:05 AM
sdardis edited edge metadata.

Ping and rebase. Dropped the changes to the ELF header flags. Those will be handled in a separate patch.

I get one reject with this review that I'm unsure how to resolve at the moment

% cat ./lib/Target/Mips/MipsISelLowering.cpp.rej
@@ -1699,7 +1702,7 @@
       MachinePointerInfo::getJumpTable(DAG.getMachineFunction()), MemVT);
   Chain = Addr.getValue(1);

-  if (isPositionIndependent() || ABI.IsN64()) {
+  if (isPositionIndependent()) {
     // For PIC, the sequence is:
     // BRIND(load(Jumptable + index) + RelocBase)
     // RelocBase can be JumpTable, GOT or some sort of global base.

That reject can be handled by removing the || ABI.IsN64() I believe. I'll take a look in a bit.

joerg added a subscriber: joerg.Nov 21 2016, 2:53 PM

LowerBR_JT no longer exists. The relevant codegen is always position-independent now, but using generic logic.

sdardis updated this revision to Diff 79106.Nov 23 2016, 9:04 AM
sdardis edited edge metadata.

Rebase and ping.

joerg added inline comments.Nov 23 2016, 1:44 PM
lib/Target/Mips/MipsISelLowering.cpp
3693 ↗(On Diff #79106)

Please leave this alone. The trade-off is intentional and should stay. If anything, it should be further refined to default to EK_GPRel32BlockAddress, but it should certainly not use absolute labels.

sdardis updated this revision to Diff 79193.Nov 24 2016, 1:20 AM
sdardis edited edge metadata.

Update jump table encodings, add comment about size.

zoran.jovanovic accepted this revision.Jan 1 2017, 4:52 PM
zoran.jovanovic added a reviewer: zoran.jovanovic.
zoran.jovanovic added a subscriber: zoran.jovanovic.

LGTM with a few nits.

lib/Target/Mips/Mips64InstrInfo.td
515 ↗(On Diff #79193)

Nit: Is this definition necessary?
If it is needed for micromips pre-r6 than it can be moved to MicroMipsInstrInfo.td.

lib/Target/Mips/MipsAsmPrinter.cpp
703 ↗(On Diff #79193)

Is this condition also affected by the change?

test/CodeGen/Mips/blockaddr.ll
38 ↗(On Diff #79193)

Nit: Tab between mnemonic and operands description should be replaced with spaces.

sdardis updated this revision to Diff 83401.Jan 6 2017, 12:31 PM
sdardis edited edge metadata.
sdardis marked 3 inline comments as done.

Updated to reflect reviewer's comments. Also removed assumption that N64 uses relative jump tables exclusively.

@seanbruno Want to give this a try?

lib/Target/Mips/Mips64InstrInfo.td
515 ↗(On Diff #79193)

It's not. It's covered by the multiclass below.

lib/Target/Mips/MipsAsmPrinter.cpp
703 ↗(On Diff #79193)

Good catch. It requires !isPositionIndependant() && STI.hasSym32(), as pic0 only applies when symbols are 32 bit according to GCC.

sdardis planned changes to this revision.Jan 10 2017, 11:00 AM

I need to refine the logic a bit further after some testing due to interactions with the CPIC ABI extension and the -mabicalls option.

I shouldn't see errors like this right? I didn't see any change before/after this patch to resolve this:

numa_setaffinity.pico: In function `err':
(.text+0x18): relocation truncated to fit: R_MIPS_PC16 against `__cerror'
numa_getaffinity.pico: In function `err':
(.text+0x18): relocation truncated to fit: R_MIPS_PC16 against `__cerror'
procctl.pico: In function `err':
(.text+0x18): relocation truncated to fit: R_MIPS_PC16 against `__cerror'
aio_mlock.pico: In function `err':
(.text+0x18): relocation truncated to fit: R_MIPS_PC16 against `__cerror'
chflagsat.pico: In function `err':
(.text+0x18): relocation truncated to fit: R_MIPS_PC16 against `__cerror'
connectat.pico: In function `err':
(.text+0x18): relocation truncated to fit: R_MIPS_PC16 against `__cerror'

Are you seeing the warning message "warning: linking PIC files with non-PIC files" as well? I have a WIP clang patch that is probably required.

seanbruno accepted this revision.Jan 19 2017, 7:46 AM

Are you seeing the warning message "warning: linking PIC files with non-PIC files" as well? I have a WIP clang patch that is probably required.

No. It looks like I need a temporary hack for now. There are plans on doing something different in the near future.

https://github.com/freebsd/freebsd/commit/e865e40a09cffd0b5a00952bef6457115d581f3c

That quick hack is a bit broad, but thanks for pointing that out, as I've found that current macro isn't shared across OSes, and there's another macro __mips_abicalls which LLVM should define but doesn't.

seanbruno added a comment.EditedJan 19 2017, 9:16 AM

Are you seeing the warning message "warning: linking PIC files with non-PIC files" as well? I have a WIP clang patch that is probably required.

Ah, I finally did hit this, do you want me to test something or other?

--- adjkerntz.full ---
/home/sbruno/clang/build/bin/clang -Wno-typedef-redefinition -Wno-pointer-sign -Wno-expansion-to-defined -Wno-address-of-packed-member -target mips64-unknown-freebsd12.0 --sysroot=/var/tmp/sbru
no/mips.mips64/home/sbruno/mips64-clang/tmp -B/var/tmp/sbruno/mips.mips64/home/sbruno/mips64-clang/tmp/usr/bin -O -pipe -G0 -EB -msoft-float -g -std=gnu99 -Wsystem-headers -Wall -Wno-format-y2k
 -W -Wno-unused-parameter -Wstrict-prototypes -Wmissing-prototypes -Wpointer-arith -Wreturn-type -Wcast-qual -Wwrite-strings -Wswitch -Wshadow -Wunused-parameter -Wcast-align -Wchar-subscripts
-Winline -Wnested-externs -Wredundant-decls -Wold-style-definition -Wno-pointer-sign -Wmissing-variable-declarations -Wthread-safety -Wno-empty-body -Wno-string-plus-int -Wno-unused-const-varia
ble -Qunused-arguments  -EB -o adjkerntz.full adjkerntz.o
/var/tmp/sbruno/mips.mips64/home/sbruno/mips64-clang/tmp/usr/bin/ld: /var/tmp/sbruno/mips.mips64/home/sbruno/mips64-clang/tmp/usr/lib/libgcc_s.so: warning: linking PIC files with non-PIC files
/var/tmp/sbruno/mips.mips64/home/sbruno/mips64-clang/tmp/usr/lib/crt1.o: In function `__start':
/home/sbruno/mips64-clang/lib/csu/mips/crt1.c:(.text+0x10c): relocation truncated to fit: R_MIPS_26 against `atexit@@FBSD_1.0'
/home/sbruno/mips64-clang/lib/csu/mips/crt1.c:(.text+0x11c): relocation truncated to fit: R_MIPS_26 against `_init_tls@@FBSD_1.0'
/home/sbruno/mips64-clang/lib/csu/mips/crt1.c:(.text+0x158): relocation truncated to fit: R_MIPS_26 against `atexit@@FBSD_1.0'
/home/sbruno/mips64-clang/lib/csu/mips/crt1.c:(.text+0x2cc): relocation truncated to fit: R_MIPS_26 against `exit@@FBSD_1.0'
adjkerntz.o: In function `main':
/home/sbruno/mips64-clang/sbin/adjkerntz/adjkerntz.c:94: relocation truncated to fit: R_MIPS_26 against `getopt@@FBSD_1.0'
/home/sbruno/mips64-clang/sbin/adjkerntz/adjkerntz.c:94: relocation truncated to fit: R_MIPS_26 against `getopt@@FBSD_1.0'
/home/sbruno/mips64-clang/sbin/adjkerntz/adjkerntz.c:94: relocation truncated to fit: R_MIPS_26 against `getopt@@FBSD_1.0'
/home/sbruno/mips64-clang/sbin/adjkerntz/adjkerntz.c:115: relocation truncated to fit: R_MIPS_26 against `access@@FBSD_1.0'
/home/sbruno/mips64-clang/sbin/adjkerntz/adjkerntz.c:121: relocation truncated to fit: R_MIPS_26 against `sigemptyset@@FBSD_1.0'
/home/sbruno/mips64-clang/sbin/adjkerntz/adjkerntz.c:122: relocation truncated to fit: R_MIPS_26 against `sigemptyset@@FBSD_1.0'
/home/sbruno/mips64-clang/sbin/adjkerntz/adjkerntz.c:123: additional relocation overflows omitted from the output
clang-4.0: error: linker command failed with exit code 1 (use -v to see invocation)
*** [adjkerntz.full] Error code 1

Not right now regarding additional testing. This patch requires some more changes. I'll repost this patch once I've produced a testsuite clean run. The clang patch for handling will follow shortly afterwards, along with a patch for defining the appropriate macros for -mabicalls.

I believe once all three are posted we'd be in a better position to take stock of the situation.

test/CodeGen/Mips/tailcall/tailcall.ll
295 ↗(On Diff #83401)

Note to self: this should be PIC64:

sdardis updated this revision to Diff 85386.Jan 23 2017, 7:39 AM

Cleaned up a broken test, and fixed the flags in the elf header when doing CPIC/PIC or static.

When the N64 abi is used with the static relocation model and symbols are 64 bit, we always enable the 'noabicalls' feature. This matches GCC's output.

This revision is now accepted and ready to land.Jan 23 2017, 7:39 AM

Retested and it looks good over here in FreeBSD land.

This revision was automatically updated to reflect the committed changes.