I'm glad to see this is gone. I was just about to file a bug report that this code used to incur a deadlock risk when unw_step was used from a signal handler since macOS 10.9.0 on x86_64, since Apple had removed the other side of this code circa keymgr-28, so it now had been attempting to call malloc. (I'd observed this deadlock occur in testing, and implemented a work around for it downstream, in https://github.com/JuliaLang/julia/pull/37101/commits/fad04d39d592d8e0fcbfba439e8157f582bbc850#diff-86a37833b03dbe7f0874e30469cb1d46R93-R102)

Does that mean I should commit it there also, or how does that process work?

review

Thanks, that's a great reduction. I'll fix this shortly.

Merged as 19f01a484760d9a29f4f414b30da110dd6550191, e244f86f4dfd9e8982940d09294af522d3809d7f, and 2c7a07b59d5da54eba8e3e030e1cc040a88ecf58

We emit an offset table of the form:

ptrdiff_t symbol_offset = &f2 - &f1;

since we know this should normally be folded to a constant in the object file. This is normally rejected by a C compiler ("error: initializer element is not a compile-time constant", pointing to the f2 symbol) but LLVM accepts it. It used to be handled here (since the difference is a constant known here), though it seems the mingw-w64 linker later corrupts this expression.

I thought about that, but I didn't think it felt like it needed to. If the cwd is readonly, it'll just silently turn itself off.

formatting

Split commit into two pieces, each more clearly NFCI

ping. With the other commits split out and landed, this is now much smaller at moving the isNonIntegralPointerType check into the correct place. This is still good to go from my perspective.

bump: this should be ready to review now.

bump? please review/merge this bugfix?

bump

Bump again? It's just a tiny bugfix that reames told me is needed (https://reviews.llvm.org/D59730?id=196652#inline-570528)

bump again? (I don't have commit rights.)

bump.

Friendly bump.

The added assertion will detect this:

[109/110] Running lit suite /data/vtjnash/julia/deps/srccache/llvm-svn/test/Transforms                                                                                       [19/519]
FAIL: LLVM :: Transforms/InstSimplify/vector_ptr_bitcast.ll (2527 of 4814)
******************** TEST 'LLVM :: Transforms/InstSimplify/vector_ptr_bitcast.ll' FAILED ********************
Script:
--
: 'RUN: at line 1';   /data/vtjnash/julia/deps/scratch/llvm-svn/build_Release+Asserts/bin/opt -S -instsimplify < /data/vtjnash/julia/deps/srccache/llvm-svn/test/Transforms/InstSimpl
ify/vector_ptr_bitcast.ll | /data/vtjnash/julia/deps/scratch/llvm-svn/build_Release+Asserts/bin/FileCheck /data/vtjnash/julia/deps/srccache/llvm-svn/test/Transforms/InstSimplify/vec
tor_ptr_bitcast.ll
--
Exit Code: 2

[GVN] teach ConstantFolding correct handling of non-integral addrspace casts

[GVN] teach ConstantFolding correct handling of non-integral addrspace casts

Bump. Fixed the other bug in handling vectors that you noted, and added an assert to catch that misuse.

FoldBitCast was being instructed to create invalid IR in several test cases. We happened to fold away them away before the verifier noticed, so this adds an explicit check, and corrects the caller FoldReinterpretLoadFromConstPtr to form valid IR when handling vectors of pointers with this method.

bump. please review, thanks!

@loladiro can you land this for me?

@loladiro bump. It sounds like this was approved, but it doesn't apply cleanly now.

Is this OK to land?

remove change that could result in a functional difference

Thanks @reames, I appreciate what you've already done here. I had started this a long time ago, where more of it was a functional change, then discovered you had recently already made most of the essential fixes and written the necessary tests. I've changed this into a stack of 3 commits that touch slightly different areas.

Split out part of the changes into two parent commits.

the warnings in that test are from a different, related bug. An approximate correction for that is:

diff --git a/lib/CodeGen/AsmPrinter/DIE.cpp b/lib/CodeGen/AsmPrinter/DIE.cpp
index a328851..73977f2 100644
--- a/lib/CodeGen/AsmPrinter/DIE.cpp
+++ b/lib/CodeGen/AsmPrinter/DIE.cpp
@@ -495,7 +495,8 @@ void DIELabel::EmitValue(const AsmPrinter *AP, dwarf::Form Form) const {
   AP->EmitLabelReference(Label, SizeOf(AP, Form),
                          Form == dwarf::DW_FORM_strp ||
                              Form == dwarf::DW_FORM_sec_offset ||
-                             Form == dwarf::DW_FORM_ref_addr);
+                             Form == dwarf::DW_FORM_ref_addr ||
+                             Form == dwarf::DW_FORM_data4);
 }

address review comments

This updates the ref_addr_relocation test to have better coverage of the various combinations of DWARF versions and architectures. Because the debug info framework in LLVM expects that .secrel32 .debug_info + offset exists (and LLVM also tests that the debug info isn't using labels that would be supported everywhere: "test that we don't create useless labels"), this also makes that a real relocation type and handles the printing and parsing of it.

bump

I couldn't get Keno's testcase to work for me (it shows the patch, but not the bug), so I re-extracted it with bugpoint and simplified it a bit more by hand.

I don't think the correct approach is to change the definition of this instruction to no longer implicitly define "CARRY". The correct approach would probably be for it to implicitly define both of the special registers that it sets (CARRY, CR0).

full context diff

it looks like the other disassembler targets also ignore Address and simply read the first N bytes from "Bytes", so i assume that was an intentional change in llvm3.5?

i don't know how to reduce the test case, but in the attached IR, the "loop-reduce" pass does an invalid transform for the computation of %57 from %25 from %"#s13.0"

@rnk I have updated the diff based on your comments (although I did not manage to locate a copy of clang-format). As suggested, I strengthened all of the conditional checks to assertions. The original code to retest the conditional was copied from the ARM code example and was not needed here.

reformat per comments