- User Since
- Jun 19 2015, 4:53 AM (140 w, 1 d)
This works as expected with your corresponding libcxx patch. However @arichardson 's D42139 does similar things for libcxx. We should wait for that to land, and ensure that this patch is rebased against that.
Thu, Feb 22
Hi Jonas, I've regenerated the tests in rL325770 and the changes seem ok. The main difference I'm seeing is some moves are being eliminated, but in a few cases in test/CodeGen/Mips/analyzebranch.ll the earlier move prevents a delay slot being filled but otherwise looks ok.
Wed, Feb 21
LGTM, some nits inlined.
Thanks for the review.
Have I missed anything important?
Tue, Feb 20
Fixed jump encodings used in the long branch pass.
Added -verify-machineinstrs to the relevant ll tests.
Mon, Feb 19
Sat, Feb 17
PS: We should also get some more Simones as well :)
LGTM. Thanks for the clean-up.
Fri, Feb 16
LGTM. Add a comment to test file, describing the purpose of the test.
Thu, Feb 15
Tue, Feb 13
Yes. Something like LIBUNWIND_TEST_CFLAGS mapping to config.test_cflags, which libunwind/test/libunwind/test/config.py then appends when building the tests.
Mon, Feb 12
I'll address the size regression after this lands. It's an odd enough case that I don't feel its worth holding this patch up.
The only thing this needs now is to get correct testing support. Could you add support for passing down to the configuration script an additional set of cflags like compiler-rt and libomp do (as a separate patch)? If you look at libomp, you'll see LIBOMP_TEST_CFLAGS defined in the cmake build system and threaded through various places so the built tests can have the correct options. Without supporting that we get a mixed set of objects, which isn't correct.
Fri, Feb 9
Thu, Feb 8
I am not sure what the answer is. There's a slightly different issue in that returning the contents of a floating point register on MIPS and expecting it to be a double is not necessarily correct. For a 64bit FPU, the result of lwc1/mtc1 leaves the upper 32bits of an floating point register as UNPREDICTABLE by the architecture definition. It's entirely possible for the application being unwound to be operating on single precision values, but the contents of the register in it's entirety are a signalling nan.
Ok, I've found my issue. inttypes.h in libcxx include_next's <inttypes.h>, which on my debian linux systems has the include chain <features.h>, <stubs.h>, <sgidefs.h>. <sgidefs.h> "helpfully" provides #defines of the various _ABIXXX macros, which normally the compiler defines depending on the ABI. The include chain for other parts of libunwind differ which means src/UnwindLevel1-gcc-ext.c had a different definition of the stuct unw_context_t, which was the version defined for O32. As GCC and clang laid out the stack differently for each ABI differently the bug was masked. However for N32 at O3, the layout was such that the unwind context in the locals area was right below the saved registers for _Unwind_Backtrace which trashed older saved values, then triggered a SIGSEGV on return due to reloading the saved contents of the $lo into $ra in my case.
Wed, Feb 7
rLLD324468 Covers this?
LGTM with two inline nits addressed.
Tue, Feb 6
Fri, Feb 2
It appears this also fixes a machine verifier failure in test/CodeGen/AMDGPU/r600.work-item-intrinsics.ll, but I am unsure on the correctness of the changed assembly output. +CC'ing Jan Vesely who provided the test.
Tue, Jan 30
Please update the title with [SelectionDAG] tag and description to reflect that this is now a selection dag patch. Also, can you restore the code to where it previously was, I was incorrect about that, as it'd just make the code harder to follow in terms of handling virtual and physical registers in the same function.
This patch isn't correct, as you're not passing the pointer to the callee or filling the rest of the argument registers with the object being passed. This results in the callee and caller differing in their interpretation of the arguments. The following test case shows this behaviour:
Marking this as changes required to clear up any confusion.
Mon, Jan 29
During review, I spotted that the left/right eva stores and loads are not in the instruction mapping tables and that they have incorrect DAG patterns associated with them but that can be submitted as a separate patch.
+CC'ing @MatzeB for additional insight on what needs to be changed.
Fri, Jan 26
I'm fine with leaving the code in the test-suite as is (no need for this followup patch).
Ok, hold off on committing this for now, I want to check this, as I'd missed the debuginfo tests when reviewing this. Good catch though.
Anyway let's leave it as is, if it breaks the build on your side.
I've seen this patch in the CHERI fork, thanks for upstreaming it. Some nits inlined and can you use update_llc_test_checks.py to generate the CHECK lines?
Jan 25 2018
Update as per @MatzeB 's comment, checking before commit showed it broke the build on OSX.
Yes, adding them manually is fine. We haven't implemented support for _builtin_prefetch or _builtin_mips_cache, so that's the only way to test them.
Jan 24 2018
Thanks, will fix the nit on commit.
Jan 23 2018
Jan 22 2018
Jan 19 2018
Jan 12 2018
I just checked both my qemu copy and on my mips64 machine and it seems to be a a copy / paste error. Reposting here directly from my machines:
Um, I now appear to be getting different results for running under QEMU doing it the proper way. I was previously rebuilding the failing test by hand and running under qemu. I don't believe I changed anything important, I'll have to take a longer look.
This was libunwind's test suite:
Sorry for the delay in getting back to this, but testing this by building it explicitly for N32 (I built a full N32 compiler + libunwind and modified the test setup py to compile for N322) and I'm getting crashes on the return path in _Unwind_Backtrace.
Jan 10 2018
Two additional nits to the ones inlined, rather that use the term 'illegal' say 'unsupported', this goes for the title as well as the test file name.
Jan 9 2018
Right now I do not know why llvm-objdump does not show a correct instruction instead of "<unknown>". By the way GNU objdump shows "swc3 $15,-4113(ra)" for "efefefef".
Jan 8 2018
Can you provide a test case for this? You can generate a .mir file with the stop-after option of llc, then modify by hand the stores and loads to use the MIPS eva instructions. The test case should then use start-after= with -mattr=+micromips, produce an object file, then disassemble the object for and check that the resulting object file contains only micromips instructions.
/Users/simon/dev/llvm/llvm/lib/Target/Mips/Disassembler/MipsDisassembler.cpp:1541:21: warning: unused function 'DecodeLoadByte9' [-Wunused-function]
static DecodeStatus DecodeLoadByte9(MCInst &Inst, ^ /Users/simon/dev/llvm/llvm/lib/Target/Mips/Disassembler/MipsDisassembler.cpp:1645:21: warning: unused function 'DecodeStoreEvaOpMM' [-Wunused-function] static DecodeStatus DecodeStoreEvaOpMM(MCInst &Inst,
Jan 6 2018
Some small nits inlined, otherwise LGTM. Please wait a few working days to let others chime in if they have suggestions before committing.
Jan 2 2018
Thanks for doing this, @fhahn.
I missed updating some tests.