Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump table optimization #3

Merged

Conversation

milica-lazarevic
Copy link
Collaborator

Currently, LLVM for nanomips uses jumptables with 4-byte entries that are absolute addresses of target labels. This patchset resolves it in the following way. First, we're emitting 1-byte entries unconditionally. Then, we're introducing a new MIR pass that checks whether it is okay to compress jump table entries. The third patch introduces the mno-jump-table-opt option that enables reverting to absolute jump tables.

clang/include/clang/Driver/Options.td Outdated Show resolved Hide resolved
llvm/lib/Target/Mips/MipsAsmPrinter.cpp Outdated Show resolved Hide resolved
llvm/lib/Target/Mips/MipsAsmPrinter.cpp Show resolved Hide resolved
llvm/lib/Target/Mips/MipsISelLowering.cpp Show resolved Hide resolved
llvm/lib/Target/Mips/MipsTargetMachine.cpp Show resolved Hide resolved
llvm/lib/Target/Mips/NanoMipsCompressJumpTables.cpp Outdated Show resolved Hide resolved
llvm/lib/Target/Mips/NanoMipsCompressJumpTables.cpp Outdated Show resolved Hide resolved
llvm/lib/Target/Mips/NanoMipsCompressJumpTables.cpp Outdated Show resolved Hide resolved
A custom lowering method for BR_JT ISDOpcode has been added. We're
emitting a MipsISD::BR_JT node with new pseudo instruction
LoadJumpTableOffset as a destination. For now, we're setting the entry
size to 1B for every jump table leaving the external assembler to decide
whether it can be done. We're also emitting relocation directive for
that purpose. Each jump table starts with a .jumptable directive.
@milica-lazarevic milica-lazarevic force-pushed the jump-table-opt branch 2 times, most recently from efcca81 to 9747030 Compare March 17, 2023 16:22
NanoMipsCompressJumpTables optimization pass should decide
if there's room for optimization. Before pass runs, the entry size for
each table is set to 4B.
An architecture-specific option mno-jump-table-opt has been added. When
set, the command line argument mno-jump-table-opt disables jump table
optimization. Therefore, table entries are fixed-size.
@cme
Copy link

cme commented Mar 21, 2023

Can we set the target branch for this to nanomips-llvm13-submission so we can merge it there for interal testing, please? (And we'll then merge to nanomips-llvm13)

@milica-lazarevic milica-lazarevic changed the base branch from nanomips-llvm13 to nanomips-llvm13-submission March 21, 2023 12:38
@cme cme merged commit b61cf61 into MediaTek-Labs:nanomips-llvm13-submission Mar 23, 2023
cme pushed a commit that referenced this pull request Sep 18, 2023
We experienced some deadlocks when we used multiple threads for logging
using `scan-builds` intercept-build tool when we used multiple threads by
e.g. logging `make -j16`

```
(gdb) bt
#0  0x00007f2bb3aff110 in __lll_lock_wait () from /lib/x86_64-linux-gnu/libpthread.so.0
#1  0x00007f2bb3af70a3 in pthread_mutex_lock () from /lib/x86_64-linux-gnu/libpthread.so.0
#2  0x00007f2bb3d152e4 in ?? ()
#3  0x00007ffcc5f0cc80 in ?? ()
#4  0x00007f2bb3d2bf5b in ?? () from /lib64/ld-linux-x86-64.so.2
#5  0x00007f2bb3b5da27 in ?? () from /lib/x86_64-linux-gnu/libc.so.6
#6  0x00007f2bb3b5dbe0 in exit () from /lib/x86_64-linux-gnu/libc.so.6
#7  0x00007f2bb3d144ee in ?? ()
#8  0x746e692f706d742f in ?? ()
#9  0x692d747065637265 in ?? ()
#10 0x2f653631326b3034 in ?? ()
#11 0x646d632e35353532 in ?? ()
#12 0x0000000000000000 in ?? ()
```

I think the gcc's exit call caused the injected `libear.so` to be unloaded
by the `ld`, which in turn called the `void on_unload() __attribute__((destructor))`.
That tried to acquire an already locked mutex which was left locked in the
`bear_report_call()` call, that probably encountered some error and
returned early when it forgot to unlock the mutex.

All of these are speculation since from the backtrace I could not verify
if frames 2 and 3 are in fact corresponding to the `libear.so` module.
But I think it's a fairly safe bet.

So, hereby I'm releasing the held mutex on *all paths*, even if some failure
happens.

PS: I would use lock_guards, but it's C.

Reviewed-by: NoQ

Differential Revision: https://reviews.llvm.org/D118439

(cherry picked from commit d919d02)
cme pushed a commit that referenced this pull request Sep 18, 2023
Change https://reviews.llvm.org/D140059 exposed the following crash in
Z3Solver, where bit widths were not checked consistently with that
change. This change makes the check consistent, and fixes the crash.

```
clang: <root>/llvm/include/llvm/ADT/APSInt.h:99:
  int64_t llvm::APSInt::getExtValue() const: Assertion
  `isRepresentableByInt64() && "Too many bits for int64_t"' failed.
...
Stack dump:
0. Program arguments: clang -cc1 -internal-isystem <root>/lib/clang/16/include
  -nostdsysteminc -analyze -analyzer-checker=core,unix.Malloc,debug.ExprInspection
  -analyzer-config crosscheck-with-z3=true -verify reproducer.c

 #0 0x00000000045b3476 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int)
  <root>/llvm/lib/Support/Unix/Signals.inc:567:22
 #1 0x00000000045b3862 PrintStackTraceSignalHandler(void*)
  <root>/llvm/lib/Support/Unix/Signals.inc:641:1
 #2 0x00000000045b14a5 llvm::sys::RunSignalHandlers()
  <root>/llvm/lib/Support/Signals.cpp:104:20
 #3 0x00000000045b2eb4 SignalHandler(int)
  <root>/llvm/lib/Support/Unix/Signals.inc:412:1
 ...
 #9 0x0000000004be2eb3 llvm::APSInt::getExtValue() const
  <root>/llvm/include/llvm/ADT/APSInt.h:99:5
  <root>/llvm/lib/Support/Z3Solver.cpp:740:53
  clang::ASTContext&, clang::ento::SymExpr const*, llvm::APSInt const&, llvm::APSInt const&, bool)
  <root>/clang/include/clang/StaticAnalyzer/Core/PathSensitive/SMTConv.h:552:61
```

Reviewed By: steakhal

Differential Revision: https://reviews.llvm.org/D142627

(cherry picked from commit f027dd5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants