[PATCH] D66255: [WebAssembly] Correctly handle va_arg of zero-sized structures

This revision was automatically updated to reflect the committed changes. Closed by commit rL369042: [WebAssembly] Correctly handle va_arg of zero-sized structures (authored by quantum, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D66255: [WebAssembly] Correctly handle va_arg of zero-sized structures

quantum added inline comments. Comment at: clang/test/CodeGen/wasm-varargs.c:104 + +struct S test_zero_size_struct(char *fmt, ...) { + va_list va; dschuff wrote: > This should maybe be called "test_empty_struct" since its size is actually 1 > and not zero. Done

[PATCH] D66255: [WebAssembly] Correctly handle va_arg of zero-sized structures

quantum updated this revision to Diff 215272. quantum marked 6 inline comments as done. quantum added a comment. Address review feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66255/new/ https://reviews.llvm.org/D66255 Files: clang/lib/C

[PATCH] D66255: [WebAssembly] Correctly handle va_arg of zero-sized structures

quantum created this revision. quantum added reviewers: dschuff, tlively, sbc100. Herald added subscribers: cfe-commits, sunfish, aheejin, jgravelle-google. Herald added a project: clang. D66168 passes size 0 structs indirectly, while the wasm backend expects it

[PATCH] D66168: [WebAssembly] Make clang emit correct va_arg code for structs

This revision was automatically updated to reflect the committed changes. Closed by commit rL368750: [WebAssembly] Make clang emit correct va_arg code for structs (authored by quantum, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit:

[PATCH] D66168: [WebAssembly] Make clang emit correct va_arg code for structs

quantum created this revision. quantum added reviewers: tlively, sbc100, kripken, aheejin. Herald added subscribers: cfe-commits, sunfish, jgravelle-google, dschuff. Herald added a project: clang. In the WebAssembly backend, when lowering variadic function calls, non-single member aggregate type a

[PATCH] D65684: [WebAssembly] Lower ASan constructor priority on Emscripten

This revision was automatically updated to reflect the committed changes. Closed by commit rL368101: [WebAssembly] Lower ASan constructor priority on Emscripten (authored by quantum, committed by ). Changed prior to commit: https://reviews.llvm.org/D65684?vs=213720&id=213730#toc Repository:

[PATCH] D65684: [WebAssembly] Lower ASan constructor priority on Emscripten

quantum updated this revision to Diff 213720. quantum added a comment. Herald added a project: clang. Herald added a subscriber: cfe-commits. Add constructor priority test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65684/new/ https://reviews.llv

[PATCH] D65028: [WebAssembly] Compute and export TLS block alignment

This revision was automatically updated to reflect the committed changes. Closed by commit rL366624: [WebAssembly] Compute and export TLS block alignment (authored by quantum, committed by ). Changed prior to commit: https://reviews.llvm.org/D65028?vs=210930&id=210934#toc Repository: rL LLVM

[PATCH] D65028: [WebAssembly] Compute and export TLS block alignment

quantum created this revision. quantum added reviewers: tlively, aheejin, sbc100, sunfish, alexcrichton. Herald added subscribers: llvm-commits, cfe-commits, hiraditya, jgravelle-google, dschuff. Herald added projects: clang, LLVM. Add immutable WASM global `__tls_align` which stores the alignmen

[PATCH] D64949: [WebAssembly] Fix __builtin_wasm_tls_base intrinsic

quantum updated this revision to Diff 210675. quantum added a comment. Add a test with a memory dependency on __builtin_wasm_tls_base. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64949/new/ https://reviews.llvm.org/D64949 Files: clang/include/

[PATCH] D64949: [WebAssembly] Fix __builtin_wasm_tls_base intrinsic

This revision was automatically updated to reflect the committed changes. Closed by commit rL366499: [WebAssembly] Fix __builtin_wasm_tls_base intrinsic (authored by quantum, committed by ). Changed prior to commit: https://reviews.llvm.org/D64949?vs=210675&id=210676#toc Repository: rL LLVM

[PATCH] D64949: [WebAssembly] Fix __builtin_wasm_tls_base intrinsic

quantum created this revision. quantum added reviewers: tlively, aheejin, sbc100, sunfish. Herald added subscribers: llvm-commits, cfe-commits, hiraditya, jgravelle-google, dschuff. Herald added projects: clang, LLVM. Properly generate the outchain for the `__builtin_wasm_tls_base` intrinsic. Al

[PATCH] D64900: [WebAssembly] Implement __builtin_wasm_tls_base intrinsic

This revision was automatically updated to reflect the committed changes. Closed by commit rL366475: [WebAssembly] Implement __builtin_wasm_tls_base intrinsic (authored by quantum, committed by ). Changed prior to commit: https://reviews.llvm.org/D64900?vs=210469&id=210632#toc Repository: rL

[PATCH] D64900: [WebAssembly] Implement __builtin_wasm_tls_base intrinsic

quantum added a comment. In D64900#1590715 , @aheejin wrote: > What does this return when `__wasm_init_tls` has not been called? It returns `0`, which I think is fine. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.l

[PATCH] D64900: [WebAssembly] Implement __builtin_wasm_tls_base intrinsic

quantum created this revision. quantum added reviewers: tlively, aheejin, sbc100. Herald added subscribers: llvm-commits, cfe-commits, sunfish, hiraditya, jgravelle-google, dschuff. Herald added projects: clang, LLVM. Add `__builtin_wasm_tls_base` so that LeakSanitizer can find the thread-local b

[PATCH] D64537: [WebAssembly] Implement thread-local storage (local-exec model)

quantum added a comment. In D64537#1588699 , @dschuff wrote: > I had a reply that got eaten here, so I'm going to keep trolling you on your > CL since we don't have a design doc for this. > The `offset` field of a data segment initializer can be a `globa

[PATCH] D64537: [WebAssembly] Implement thread-local storage (local-exec model)

quantum updated this revision to Diff 210188. quantum added a comment. Disable atomics when TLS is stripped Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64537/new/ https://reviews.llvm.org/D64537 Files: clang/include/clang/Basic/BuiltinsWebAsse

[PATCH] D64537: [WebAssembly] Implement thread-local storage (local-exec model)

This revision was automatically updated to reflect the committed changes. Closed by commit rL366272: [WebAssembly] Implement thread-local storage (local-exec model) (authored by quantum, committed by ). Changed prior to commit: https://reviews.llvm.org/D64537?vs=210188&id=210191#toc Repository

[PATCH] D64537: [WebAssembly] Implement thread-local storage (local-exec model)

quantum updated this revision to Diff 210187. quantum added a comment. Remove extra braces Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64537/new/ https://reviews.llvm.org/D64537 Files: clang/include/clang/Basic/BuiltinsWebAssembly.def clang/

[PATCH] D64537: [WebAssembly] Implement thread-local storage (local-exec model)

quantum updated this revision to Diff 210184. quantum added a comment. Removed the trailing `.`. I would add a test to make sure things work without `-fdata-sections`, but `this->Options.DataSections = true;` is hard-coded in `WebAssemblyTargetMachine.cpp`. Repository: rG LLVM Github Monorep

[PATCH] D64537: [WebAssembly] Implement thread-local storage (local-exec model)

quantum updated this revision to Diff 210166. quantum added a comment. More fine-grainted stripping. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64537/new/ https://reviews.llvm.org/D64537 Files: clang/include/clang/Basic/BuiltinsWebAssembly.de

[PATCH] D64537: [WebAssembly] Implement thread-local storage (local-exec model)

quantum added a comment. In D64537#1586809 , @aheejin wrote: > Where should we call `__wasm_init_tls`, in case within a library? Dynamic libraries are not supported by the local-exec TLS model. Support will be implemented later. The idea is that we'll h

[PATCH] D64776: [WebAssembly] Compile all TLS on Emscripten as local-exec

quantum updated this revision to Diff 209996. quantum added a comment. Herald added subscribers: cfe-commits, jfb. Herald added a project: clang. Change to use TODO instead of XXX Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64776/new/ https://rev

[PATCH] D64776: [WebAssembly] Compile all TLS on Emscripten as local-exec

quantum updated this revision to Diff 209997. quantum added a comment. Undo compressing the previous diff. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64776/new/ https://reviews.llvm.org/D64776 Files: llvm/lib/Target/WebAssembly/WebAssemblyISe

[PATCH] D64537: [WebAssembly] Implement thread-local storage (local-exec model)

quantum added a comment. In D64537#1586577 , @dschuff wrote: > Another high-level question (based just on reading the CL description): The > TLS-size intrinsic is per-function, does that mean that the tls-init function > is called for every function? are

[PATCH] D64537: [WebAssembly] Implement thread-local storage (local-exec model)

quantum updated this revision to Diff 209966. quantum added a comment. Fix duplicate end function Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64537/new/ https://reviews.llvm.org/D64537 Files: clang/include/clang/Basic/BuiltinsWebAssembly.def

[PATCH] D64537: [WebAssembly] Implement thread-local storage (local-exec model)

quantum added inline comments. Comment at: lld/test/wasm/data-segments.ll:7 ; RUN: wasm-ld -no-gc-sections --no-entry --shared-memory --max-memory=131072 %t.atomics.o -o %t.atomics.wasm -; RUN: obj2yaml %t.atomics.wasm | FileCheck %s --check-prefix ACTIVE +; RUN: obj2yaml %t.at

[PATCH] D64537: [WebAssembly] Implement thread-local storage (local-exec model)

quantum updated this revision to Diff 209953. quantum marked 16 inline comments as done. quantum added a comment. Deal with review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64537/new/ https://reviews.llvm.org/D64537 Files: clang/in

[PATCH] D64537: [WebAssembly] Implement thread-local storage (local-exec model)

quantum added inline comments. Comment at: lld/test/wasm/data-segments.ll:7 ; RUN: wasm-ld -no-gc-sections --no-entry --shared-memory --max-memory=131072 %t.atomics.o -o %t.atomics.wasm -; RUN: obj2yaml %t.atomics.wasm | FileCheck %s --check-prefix ACTIVE +; RUN: obj2yaml %t.at

[PATCH] D64537: [WebAssembly] Implement thread-local storage (local-exec model)

quantum updated this revision to Diff 209658. quantum marked 14 inline comments as done. quantum added a comment. Change per review feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64537/new/ https://reviews.llvm.org/D64537 Files: clang/i

[PATCH] D64537: [WebAssembly] Implement thread-local storage (local-exec model)

quantum updated this revision to Diff 209656. quantum added a comment. Use signed LEB128 for `i32.const`. Basically, do what D64612 did. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64537/new/ https://reviews.llv

[PATCH] D64537: [WebAssembly] Implement thread-local storage (local-exec model)

quantum added a comment. In D64537#1584069 , @dschuff wrote: > Oh, btw, any reason these have to be passive segments? Why can't we just make > them active segments and let the VM initialize them for us? These need to be passive segments so that we can i

[PATCH] D64537: [WebAssembly] Implement thread-local storage (local-exec model)

quantum added a comment. @sbc100 As for `__tls_base` being immutable, I am not sure how this will interact with dynamic linking once we implement it. On native platforms, `dlopen`ed libraries have their TLS blocks allocated on first use. What exactly happens to `dlopen`ed libraries when there a

[PATCH] D64537: [WebAssembly] Implement thread-local storage (local-exec model)

quantum updated this revision to Diff 209566. quantum added a comment. Don't show a crash report for incorrect flags to use TLS Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64537/new/ https://reviews.llvm.org/D64537 Files: clang/include/clang/B

[PATCH] D64537: [WebAssembly] Implement thread-local storage (local-exec model)

quantum updated this revision to Diff 209560. quantum marked an inline comment as done. quantum added a comment. Switch to using ISelDAGToDAG instead of ISelLowering Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64537/new/ https://reviews.llvm.org/

[PATCH] D64537: [WebAssembly] Implement thread-local storage (local-exec model)

quantum marked 4 inline comments as done. quantum added inline comments. Comment at: lld/wasm/Writer.cpp:629 + // Merge .tbss into .tdata so that they share the same offsets. + if (name.startswith(".tbss.")) +return ".tdata"; sbc100 wrote: > Maybe write thi

[PATCH] D64537: [WebAssembly] Implement thread-local storage (local-exec model)

quantum updated this revision to Diff 209535. quantum added a comment. Guard `__wasm_init_tls` and TLS globals behind a flag (currently `--shared-memory`). @tlively and I discussed offline and agreed that it's probably best to use the existing flag instead of adding another flag that needs to b

[PATCH] D64537: [WebAssembly] Implement thread-local storage (local-exec model)

quantum marked 28 inline comments as done. quantum added inline comments. Comment at: clang/include/clang/Basic/BuiltinsWebAssembly.def:33 +// Thread-local storage +TARGET_BUILTIN(__builtin_wasm_tls_size, "z", "nc", "bulk-memory") + tlively wrote: > quantum wrote

[PATCH] D64537: [WebAssembly] Implement thread-local storage (local-exec model)

quantum updated this revision to Diff 209391. quantum marked 10 inline comments as done. quantum added a comment. Apply review feedback Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64537/new/ https://reviews.llvm.org/D64537 Files: clang/include

[PATCH] D64537: [WebAssembly] Implement thread-local storage (local-exec model)

quantum updated this revision to Diff 209376. quantum added a comment. Rebased Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64537/new/ https://reviews.llvm.org/D64537 Files: clang/include/clang/Basic/BuiltinsWebAssembly.def clang/lib/CodeGen/

[PATCH] D64537: [WebAssembly] Implement thread-local storage for non-PIC cases

quantum updated this revision to Diff 209304. quantum added a comment. Treat global.get of an immutable global as a memory access for now. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64537/new/ https://reviews.llvm.org/D64537 Files: clang/incl

[PATCH] D64537: [WebAssembly] Implement thread-local storage for non-PIC cases

quantum marked 5 inline comments as done. quantum added inline comments. Comment at: clang/include/clang/Basic/BuiltinsWebAssembly.def:33 +// Thread-local storage +TARGET_BUILTIN(__builtin_wasm_tls_size, "z", "nc", "bulk-memory") + quantum wrote: > aheejin wrote:

[PATCH] D64537: [WebAssembly] Implement thread-local storage for non-PIC cases

quantum updated this revision to Diff 209274. quantum added a comment. Clean up table gen. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64537/new/ https://reviews.llvm.org/D64537 Files: clang/include/clang/Basic/BuiltinsWebAssembly.def clang/

[PATCH] D64537: [WebAssembly] Implement thread-local storage for non-PIC cases

quantum updated this revision to Diff 209271. quantum marked 6 inline comments as done. quantum added a comment. Update comments regarding multable globals. Use `CHECK-DAG`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64537/new/ https://reviews.l

[PATCH] D64537: [WebAssembly] Implement thread-local storage for non-PIC cases

quantum marked 20 inline comments as done. quantum added a comment. I'll clean up the table gen stuff. Comment at: clang/include/clang/Basic/BuiltinsWebAssembly.def:33 +// Thread-local storage +TARGET_BUILTIN(__builtin_wasm_tls_size, "z", "nc", "bulk-memory") +

[PATCH] D64537: [WebAssembly] Implement thread-local storage for non-PIC cases

quantum updated this revision to Diff 209258. quantum added a comment. Add error for non-local-exec TLS models. Fix unit tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64537/new/ https://reviews.llvm.org/D64537 Files: clang/include/clang/B

[PATCH] D64537: [WebAssembly] Implement thread-local storage for non-PIC cases

quantum marked an inline comment as done. quantum added a comment. In D64537#1579626 , @sunfish wrote: > This looks nice! > > > __wasm_init_tls(calloc(__builtin_wasm_tls_size(), 1)); > > Would it make sense to change the API contract for `__wasm_init_tls`

[PATCH] D64537: [WebAssembly] Implement thread-local storage for non-PIC cases

quantum updated this revision to Diff 209102. quantum added a comment. Fix linker warning. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64537/new/ https://reviews.llvm.org/D64537 Files: clang/include/clang/Basic/BuiltinsWebAssembly.def clang/

[PATCH] D64537: [WebAssembly] Implement thread-local storage for non-PIC cases

quantum updated this revision to Diff 209090. quantum added a comment. Fix `fail` call. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64537/new/ https://reviews.llvm.org/D64537 Files: clang/include/clang/Basic/BuiltinsWebAssembly.def clang/lib

[PATCH] D64537: [WebAssembly] Implement thread-local storage for non-PIC cases

quantum updated this revision to Diff 209084. quantum added a comment. Add tests for `__builtin_wasm_tls_size`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64537/new/ https://reviews.llvm.org/D64537 Files: clang/include/clang/Basic/BuiltinsWeb

[PATCH] D64537: [WebAssembly] Implement thread-local storage for non-PIC cases

quantum planned changes to this revision. quantum added a comment. Need to add tests for the compiler intrinsic `__builtin_wasm_tls_size`. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64537/new/ https://reviews.llvm.org/D64537 _

[PATCH] D64537: [WebAssembly] Implement thread-local storage for non-PIC cases

quantum created this revision. quantum added reviewers: tlively, aheejin, kripken, sbc100. Herald added subscribers: llvm-commits, cfe-commits, jfb, sunfish, hiraditya, jgravelle-google, dschuff. Herald added projects: clang, LLVM. quantum planned changes to this revision. quantum added a comment.

[PATCH] D63742: [WebAssembly] Implement Address Sanitizer for Emscripten

This revision was automatically updated to reflect the committed changes. Closed by commit rL364468: [WebAssembly] Implement Address Sanitizer for Emscripten (authored by quantum, committed by ). Changed prior to commit: https://reviews.llvm.org/D63742?vs=206719&id=206732#toc Repository: rL

[PATCH] D63742: [WebAssembly] Implement Address Sanitizer for Emscripten

quantum updated this revision to Diff 206719. quantum added a comment. Split wasm-ld change into D63833 . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63742/new/ https://reviews.llvm.org/D63742 Files: clang/lib/

[PATCH] D63742: [WebAssembly] Implement Address Sanitizer for Emscripten

quantum marked an inline comment as done. quantum added a comment. As for the name, I think `__global_base` matches the command line flag and makes it more clear what controls it, so I lean towards that. Comment at: lld/wasm/Writer.cpp:228 + if (WasmSym::GlobalBase) +Wasm

[PATCH] D63742: [WebAssembly] Implement Address Sanitizer for Emscripten

quantum created this revision. quantum added reviewers: tlively, aheejin. Herald added subscribers: llvm-commits, cfe-commits, sunfish, hiraditya, jgravelle-google, sbc100, dschuff. Herald added projects: clang, LLVM. This diff enables address sanitizer on Emscripten. On Emscripten, real memory

[PATCH] D62830: [WebAssembly] Support Leak Sanitizer on Emscripten

quantum added a comment. It's not urgent, so I am willing to wait a day or two for commit access. If I still can't get access by then, I'll ask someone else to commit this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62830/new/ https://reviews.l

[PATCH] D62830: [WebAssembly] Support Leak Sanitizer on Emscripten

quantum added a comment. LSan is supposed to be linked in to replace some C library functions, and does not change the generated LLVM IR code, so there is nothing to actually test in LLVM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62830/new/

[PATCH] D62830: [WebAssembly] Support Leak Sanitizer on Emscripten

quantum created this revision. quantum added reviewers: tlively, aheejin. Herald added subscribers: cfe-commits, sunfish, jgravelle-google, sbc100, dschuff. Herald added a project: clang. LSan is currently being ported to Emscripten and mostly works. Enabling the support in upstream would simpli

[PATCH] D62559: [WebAssembly] Support VPtr sanitizer for Emscripten

quantum marked 2 inline comments as done. quantum added inline comments. Comment at: clang/lib/Driver/ToolChains/WebAssembly.cpp:214 + if (getTriple().isOSEmscripten()) { +Res |= SanitizerKind::Vptr; + } aheejin wrote: > quantum wrote: > > aheejin wrote: >

[PATCH] D62559: [WebAssembly] Support VPtr sanitizer for Emscripten

quantum updated this revision to Diff 201803. quantum marked an inline comment as done. quantum added a comment. Add a test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62559/new/ https://reviews.llvm.org/D62559 Files: clang/lib/Driver/ToolChai

[PATCH] D62559: [WebAssembly] Support VPtr sanitizer for Emscripten

quantum marked 2 inline comments as done. quantum added inline comments. Comment at: clang/lib/Driver/ToolChains/WebAssembly.cpp:214 + if (getTriple().isOSEmscripten()) { +Res |= SanitizerKind::Vptr; + } aheejin wrote: > Does this mean we only support `Vptr

[PATCH] D62559: [WebAssembly] Support VPtr sanitizer for Emscripten

quantum created this revision. quantum added reviewers: tlively, aheejin. Herald added subscribers: cfe-commits, sunfish, jgravelle-google, sbc100, dschuff. Herald added a project: clang. After https://github.com/emscripten-core/emscripten/pull/8651, Emscripten supports the full UBSan runtime. Th

[PATCH] D62333: Fix unresolved symbols when linking tools/clang/unittests/Tooling/ToolingTests

quantum marked 2 inline comments as done. quantum added inline comments. Comment at: cfe/trunk/unittests/Tooling/CMakeLists.txt:4 Support TestingSupport ) thakis wrote: > The library is already up here. Why do we need it twice? We don't. It's removed in

[PATCH] D62333: Fix unresolved symbols when linking tools/clang/unittests/Tooling/ToolingTests

quantum created this revision. quantum added reviewers: tlively, aheejin. Herald added subscribers: cfe-commits, mgorny. Herald added a project: clang. Add correct cmake dependencies so that `ToolingTests` link successfully. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D62333