[PATCH] D159383: [Headers] Remove musl-related comment about NULL

sunfish added a comment. I believe wasi-sdk doesn't have this issue, because it has modifications to all headers which define NULL to always use the NULL defined in stddef.h, and it doesn't ship musl's stddef.h. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D153293: [clang][WebAssembly] support wasm32-wasi shared libraries

sunfish added a comment. This patch appears to be just the release notes and the test; is the actual driver change missing? Comment at: clang/docs/ReleaseNotes.rst:706 + limited to the Emscripten target OS and now works with other targets such as + wasm32-wasi.

[PATCH] D150803: Add a new `wasm_custom` clang attribute for marking functions.

sunfish added a comment. In D150803#4419436 , @sbc100 wrote: > Did you mean to comment on the old PR? This new PR doesn't propose either > "jspi" or "async", but adds that ability to define custom attributes.. since > that was deemed more flexible and

[PATCH] D150803: Add a new `wasm_custom` clang attribute for marking functions.

sunfish added a comment. Would it make sense to call this "jspi" in practice, rather than "async"? Even though this isn't clang's decision per se anymore, it seems like the reasoning earlier in this review would still apply. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D150803: [WebAssembly] Add a new `wasm_async` clang attribute for marking async functions.

sunfish added a comment. In D150803#4401204 , @sbc100 wrote: > In D150803#4401061 , @sunfish wrote: > >> wasm_jspi works for me. > > The problem with that is that we will also likely want to use this attribute >

[PATCH] D150803: [WebAssembly] Add a new `wasm_async` clang attribute for marking async functions.

sunfish added a comment. wasm_jspi works for me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D150803/new/ https://reviews.llvm.org/D150803 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D150803: [WebAssembly] Add a new `wasm_async` clang attribute for marking async functions.

sunfish added a comment. In D150803#4389040 , @dschuff wrote: > Hm, this is interesting because in the long term we plan to have stack > switching in wasm, which could allow for similar async behavior that JSPI > has, and could be useful in non-web

[PATCH] D125728: [WebAssembly] Update supported features in -mcpu=generic

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG1e4e2433bcd1: [WebAssembly] Update supported features in the generic CPU configuration (authored by sunfish). Changed prior to commit:

[PATCH] D75277: [WebAssembly] Remove restriction on main name mangling

sunfish added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6360 + auto *GA = llvm::GlobalAlias::create("__main_void", F); + GA->setVisibility(F->getVisibility()); +} sbc100 wrote: > sunfish wrote: > > Can this use

[PATCH] D75277: [WebAssembly] Remove restriction on main name mangling

sunfish added inline comments. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:6360 + auto *GA = llvm::GlobalAlias::create("__main_void", F); + GA->setVisibility(F->getVisibility()); +} Can this use

[PATCH] D75277: [WebAssembly] Remove restriction on main name mangling

sunfish added inline comments. Comment at: llvm/lib/Target/WebAssembly/WebAssemblyFixFunctionBitcasts.cpp:264 - } -} } Why is this code going away? This isn't Emscripten-specific code. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D125728: [WebAssembly] Update supported features in -mcpu=generic

sunfish updated this revision to Diff 431067. sunfish added a comment. Add tests for -mcpu=mvp and -mcpu=bleeding-edge. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125728/new/ https://reviews.llvm.org/D125728 Files:

[PATCH] D125728: [WebAssembly] Update supported features in -mcpu=generic

sunfish updated this revision to Diff 431064. sunfish added a comment. Add a driver test, and add release notes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D125728/new/ https://reviews.llvm.org/D125728 Files: clang/docs/ReleaseNotes.rst

[PATCH] D125728: [WebAssembly] Update supported features in -mcpu=generic

sunfish created this revision. Herald added subscribers: pmatos, asb, ecnelises, jgravelle-google, sbc100, dschuff. Herald added a project: All. sunfish requested review of this revision. Herald added a subscriber: aheejin. Herald added a project: clang. Enable nontrapping-fptoint, sign-ext,

[PATCH] D121327: Lower `@llvm.global_dtors` using `__cxa_atexit` on MachO

sunfish added a comment. In D121327#3381123 , @yln wrote: > @sunfish > Hi Dan, I hope you are still happy with this change. I didn't change any > WebAssembly tests, but rather added a new IR-level test, so all existing > WebAssembly behavior should

[PATCH] D105749: WebAssembly: Update datalayout to match fp128 ABI change

sunfish added inline comments. Comment at: llvm/test/CodeGen/WebAssembly/varargs.ll:5 -target datalayout = "e-m:e-p:32:32-i64:64-n32:64-S128" -target triple = "wasm32-unknown-unknown" +target triple = "wasm32-unknown-emscripten" It appears this change means

[PATCH] D104808: [clang][emscripten] Reduce alignof long double from 16 to 8 bytes

sunfish added a comment. Do we still intend to unify Emscripten's ABI with wasm32-unknown-unknown or wasm32-wasi eventually? This is talking a step away from that. One of the assumptions behind this is that it would be ok for malloc to be 16-byte aligned anyway, because SIMD use cases benefit

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

sunfish added inline comments. Comment at: llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.h:39 + // pointers are lowered to global.get / global.set or local.get / local.set, + // as appropriate. + WASM_ADDRESS_SPACE_MANAGED = 1 tlively wrote: >

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

sunfish added inline comments. Comment at: llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.h:39 + // pointers are lowered to global.get / global.set or local.get / local.set, + // as appropriate. + WASM_ADDRESS_SPACE_MANAGED = 1 tlively wrote: >

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

sunfish added inline comments. Comment at: llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp:1279 + if (const GlobalAddressSDNode *GA = dyn_cast(Op)) +return WebAssembly::isManagedAddressSpace(GA->getAddressSpace()); + sbc100 wrote: > sunfish wrote: >

[PATCH] D101608: [WebAssembly] Support for WebAssembly globals in LLVM IR

sunfish added inline comments. Comment at: llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.h:39 + // pointers are lowered to global.get / global.set or local.get / local.set, + // as appropriate. + WASM_ADDRESS_SPACE_MANAGED = 1 Sorry to throw more

[PATCH] D100411: [WebAssembly] Use standard intrinsics for f32x4 and f64x2 ops

sunfish added a comment. Great, thanks! And yes, switching to roundeven for both scalar and SIMD ISel sounds right to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100411/new/ https://reviews.llvm.org/D100411

[PATCH] D100411: [WebAssembly] Use standard intrinsics for f32x4 and f64x2 ops

sunfish accepted this revision. sunfish added a comment. This revision is now accepted and ready to land. This looks good to me! Could you briefly comment here on what the issue with `llvm.roundeven` is? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D89274: [WebAssembly] Use the new crt1-command.o if present.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGf9c05fc39145: [WebAssembly] Use the new crt1-command.o if present. (authored by sunfish). Changed prior to commit:

[PATCH] D89274: [WebAssembly] Use the new crt1-command.o if present.

sunfish added a comment. I don't see a way to do this with weak symbols, and an install script would be yet-another moving part that we'd have to make on end-user systems. How about this: once we reach a point where we don't support the old LLVM anymore, libc can make crt1.o be the same as

[PATCH] D89274: [WebAssembly] Use the new crt1-command.o if present.

sunfish added a comment. It's to ensure that older LLVM works with newer WASI libc, and newer clang works with older WASI libc. New-style commands require [lld support]. We can assume that if clang is updated, lld has the requisite support. That said, I'm open to other ideas here. [lld

[PATCH] D89274: [WebAssembly] Use the new crt1-command.o if present.

sunfish added a comment. Ping! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89274/new/ https://reviews.llvm.org/D89274 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D96091: [WebAssembly] Use single-threaded mode when -matomics isn't enabled.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG95da64da23ac: [WebAssembly] Use single-threaded mode when -matomics isnt enabled. (authored by sunfish). Repository: rG LLVM Github Monorepo

[PATCH] D96091: [WebAssembly] Use single-threaded mode when -matomics isn't enabled.

sunfish created this revision. sunfish added a reviewer: tlively. Herald added subscribers: ecnelises, jfb, jgravelle-google, sbc100, dschuff. sunfish requested review of this revision. Herald added subscribers: cfe-commits, aheejin. Herald added a project: clang. When the -matomics feature is

[PATCH] D89274: [WebAssembly] Use the new crt1-command.o if present.

sunfish created this revision. sunfish added a reviewer: sbc100. Herald added subscribers: cfe-commits, ecnelises, jgravelle-google, dschuff. Herald added a project: clang. sunfish requested review of this revision. Herald added a subscriber: aheejin. If crt1-command.o exists in the sysroot, the

[PATCH] D85347: [WebAssembly] Fix types in wasm_simd128.h and add tests

sunfish accepted this revision. sunfish added a comment. This revision is now accepted and ready to land. Ah, it seems the reason I didn't see this is that I wasn't using `-flax-vector-conversions=none`. Thanks for fixing this! Comment at: clang/test/Headers/wasm.cpp:1 +//

[PATCH] D85074: [WebAssembly] Use "signed char" instead of "char" in SIMD intrinsics.

sunfish updated this revision to Diff 282608. sunfish added a comment. - Update clang/test/CodeGen/builtins-wasm.c. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85074/new/ https://reviews.llvm.org/D85074 Files:

[PATCH] D85074: [WebAssembly] Use "signed char" instead of "char" in SIMD intrinsics.

sunfish created this revision. sunfish added a reviewer: tlively. Herald added subscribers: jgravelle-google, sbc100, dschuff. Herald added a project: clang. sunfish requested review of this revision. Herald added a subscriber: aheejin. This allows people to use `int8_t` instead of `char`,

[PATCH] D81688: [WebAssembly] WebAssembly doesn't support "protected" visibility

sunfish closed this revision. sunfish added a comment. Landed in 66042959590d6db9d2a12803a16476d4e3508f3f . Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D81688/new/

[PATCH] D81688: [WebAssembly] WebAssembly doesn't support "protected" visibility

sunfish created this revision. Herald added subscribers: llvm-commits, aheejin, jgravelle-google, sbc100, dschuff. Herald added projects: clang, LLVM. mplement the `hasProtectedVisibility()` hook to indicate that, like Darwin, WebAssembly doesn't support "protected" visibility. On ELF,

[PATCH] D62922: [WebAssembly] Implement "Reactor" mode

sunfish marked an inline comment as done. sunfish added inline comments. Comment at: clang/test/Driver/wasm-toolchain.c:116 +// CHECK-COMMAND: clang{{.*}}" "-cc1" {{.*}} "-o" "[[temp:[^"]*]]" +// CHECK-COMMAND: wasm-ld{{.*}}" "crt1.o" "[[temp]]" "-lc"

[PATCH] D62922: [WebAssembly] Implement "Reactor" mode

sunfish marked an inline comment as done. sunfish added inline comments. Comment at: clang/test/Driver/wasm-toolchain.c:116 +// CHECK-COMMAND: clang{{.*}}" "-cc1" {{.*}} "-o" "[[temp:[^"]*]]" +// CHECK-COMMAND: wasm-ld{{.*}}" "crt1.o" "[[temp]]" "-lc"

[PATCH] D59520: [WebAssembly] Address review comments on r352930

sunfish marked 3 inline comments as done. sunfish added a comment. I apologize again for the major delay. I've now updated the patch and addressed all of your comments. Comment at: clang/lib/Sema/SemaDecl.cpp:2594-2597 + else if (const auto *IMA = dyn_cast(Attr)) +

[PATCH] D59520: [WebAssembly] Address review comments on r352930

sunfish updated this revision to Diff 268329. sunfish added a comment. - Add tests for redeclaration behavior - Remove disabled tests (previously marked with FIXMEs) - Made the mismatch warning more informative. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D70500: [WebAssembly] Enable use of wasm-opt and LTO-enabled system libraries

sunfish added a comment. In D70500#1999268 , @bernhard wrote: > > It's for users who want smaller wasm binaries. It's not currently > > documented, though yes, it would be nice to document it. > > But how would a user even end up with wasm-opt in the

[PATCH] D70500: [WebAssembly] Enable use of wasm-opt and LTO-enabled system libraries

sunfish added a comment. In D70500#1998994 , @bernhard wrote: > Are there plans to offer a way to disable this behavior (or have it optional > in the first place)? > We'd like to run some custom processing between wasm-ld and wasm-opt which > can't

[PATCH] D62922: [WebAssembly] Implement "Reactor" mode

sunfish added a comment. This addresses the review feedback from earlier. To answer this question: > I assume that all he users -Wl args come after.. so its still possible to set > --entry on the command line? Yes, that is what happens. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D62922: [WebAssembly] Implement "Reactor" mode

sunfish updated this revision to Diff 257171. sunfish added a comment. Herald added a project: clang. Herald added a subscriber: cfe-commits. Rebase, update, add a test, and add basic error reporting. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D77908: [WebAssembly] Enable nontrapping-fptoint for `default` cpu

sunfish added a comment. Looks good! I don't have anything to add beyond Thomas' review comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77908/new/ https://reviews.llvm.org/D77908 ___

[PATCH] D76959: [WebAssembly] Import wasm_simd128.h from Emscripten

sunfish accepted this revision. sunfish added a comment. This revision is now accepted and ready to land. Cool, LGTM, with optional suggestion for signed char below: Comment at: clang/lib/Headers/wasm_simd128.h:30 +typedef long long __i64x2 __attribute__((__vector_size__(16),

[PATCH] D76959: [WebAssembly] Import wasm_simd128.h from Emscripten

sunfish added a comment. Very cool, thanks for putting this together! Comment at: clang/lib/Headers/wasm_simd128.h:10 + +#pragma once + Do you know why other clang headers, such as `lib/Headers/xmmintrin.h`, don't use `#pragma once`?

[PATCH] D76547: [WebAssembly] Add wasm-exported function attribute

sunfish added a comment. Instead of creating a new LLVM-IR-level attribute here, could you have clang translate the attribute to "wasm-export-name", to keep the LLVM-IR level simpler? Also, I myself would be more comfortable with this change if it were restricted to Emscripten for now.

[PATCH] D70700: [WebAssembly] Mangle the argc/argv `main` as `__main_argc_argv`

sunfish added a comment. @sbc100 Friendly ping :-). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70700/new/ https://reviews.llvm.org/D70700 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D59520: [WebAssembly] Address review comments on r352930

sunfish marked 10 inline comments as done. sunfish added a comment. I apologize for the extraordinary delays here; at long last, I've now addressed your feedback. Comment at: lib/Sema/SemaDeclAttr.cpp:5781-5783 +Sema::mergeImportNameAttr(Decl *D, SourceRange Range, +

[PATCH] D59520: [WebAssembly] Address review comments on r352930

sunfish updated this revision to Diff 235005. sunfish added a comment. Address review feedback. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D59520/new/ https://reviews.llvm.org/D59520 Files: clang/include/clang/Basic/DiagnosticSemaKinds.td

[PATCH] D70700: [WebAssembly] Mangle the argc/argv `main` as `__wasm_argc_argv`

sunfish updated this revision to Diff 234942. sunfish added a comment. To support a transition to the new system, temporarily define a `__main_void` alias for no-argument `main`. This allows libc to detect whether it's calling old-style or new-style main and do the right thing. Repository:

[PATCH] D70700: [WebAssembly] Mangle the argc/argv `main` as `__wasm_argc_argv`

sunfish updated this revision to Diff 234426. sunfish added reviewers: sbc100, dschuff, aheejin. sunfish added a comment. This updates the `main` vs `__main_argc_argv` patch so that it doesn't apply to Emscripten targets, following the discussion in

[PATCH] D71493: [WebAssembly] Setting export_name implies no_dead_strip

sunfish added inline comments. Comment at: llvm/lib/Target/WebAssembly/AsmParser/WebAssemblyAsmParser.cpp:725 TOut.emitExportName(WasmSym, ExportName); + Out.EmitSymbolAttribute(WasmSym, MCSA_NoDeadStrip); } It feels like this is a little

[PATCH] D70500: [WebAssembly] Enable use of wasm-opt and LTO-enabled system libraries

sunfish added a comment. In D70500#1759831 , @ilya-biryukov wrote: > I'm not an expert in driver code and how it should behave, but being > consistent with how other tools are found definitely looks much better than > special-casing a single tool. >

[PATCH] D70780: [WebAssembly] Find wasm-opt with GetProgramPath

sunfish created this revision. sunfish added reviewers: dschuff, ilya-biryukov. Herald added subscribers: cfe-commits, aheejin, jgravelle-google, sbc100. Herald added a project: clang. Instead of just searching for wasm-opt in PATH, use `GetProgramPath`, which checks the `COMPILER_PATH`

[PATCH] D70687: [WebAssembly] Add an llvm-lto path for compiler-rt.

sunfish abandoned this revision. sunfish added a comment. I've done some more experimenting with this, and it turns out not to work very well, because calls to these functions get generated after LTO runs, so I'll abandon this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D70700: [WebAssembly] Mangle the argc/argv `main` as `__wasm_argc_argv`

sunfish created this revision. Herald added subscribers: llvm-commits, dexonsmith, aheejin, jgravelle-google, sbc100, mehdi_amini, dschuff. Herald added projects: clang, LLVM. WebAssembly enforces a rule that caller and callee signatures must match. This means that the traditional technique of

[PATCH] D70687: [WebAssembly] Add an llvm-lto path for compiler-rt.

sunfish created this revision. Herald added subscribers: cfe-commits, dexonsmith, aheejin, jgravelle-google, inglorion, sbc100, mehdi_amini, dberris, dschuff. Herald added a project: clang. This allows LTO-enabled builds of compiler-rt to be used. Repository: rG LLVM Github Monorepo

[PATCH] D70500: [WebAssembly] Enable use of wasm-opt and LTO-enabled system libraries

sunfish added a comment. I've now posted https://reviews.llvm.org/D70677 which should fix the test failure when `LLVM_APPEND_VC_REV=NO` is set. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70500/new/ https://reviews.llvm.org/D70500

[PATCH] D70677: [WebAssembly] Change the llvm-lto dir to use the LLVM Version

sunfish created this revision. sunfish added reviewers: sbc100, dschuff. Herald added subscribers: cfe-commits, dexonsmith, aheejin, jgravelle-google, inglorion. Herald added a project: clang. Using the version instead of the VCS revision, which isn't available when LLVM_APPEND_VC_REV is set.

[PATCH] D70500: [WebAssembly] Enable use of wasm-opt and LTO-enabled system libraries

sunfish marked an inline comment as done. sunfish added a comment. In D70500#1757735 , @thakis wrote: > Please don't add code to the driver that runs programs off PATH. Nothing else > does this. Clang's normal `GetProgramPath` does do this:

[PATCH] D70500: [WebAssembly] Enable use of wasm-opt and LTO-enabled system libraries

sunfish added a comment. In D70500#1756011 , @dschuff wrote: > WRT the LTO directory name, there's theoretically the danger that someone > (e.g. emscripten) could be doing a rolling release of the compiler and get > invalidated within a major revision.

[PATCH] D70520: [WebAssembly] Add new `export_name` clang attribute for controlling wasm export names

sunfish added a comment. > Do you think that setting this attribute should also prevent GC? That sounds right to me. > I could split this up into clang, llvm, and lld parts if it makes reviewing > simpler. For me, it's easier to have it all together in one. In this patch, `export_name` has a

[PATCH] D70520: [WebAssembly] Add new `export_name` clang attribute for controlling wasm export names

sunfish added a comment. It appears this doesn't handle exporting an imported function yet, which is fine for now, but it would be good to issue a warning, because wasm itself is capable of representing this: void aaa(void) __attribute__((import_module("imp"), import_name("foo"),

[PATCH] D70500: [WebAssembly] Enable use of wasm-opt and LTO-enabled system libraries

sunfish marked 2 inline comments as done. sunfish added inline comments. Comment at: clang/lib/Driver/ToolChains/WebAssembly.cpp:105 +OOpt = "0"; + else if (A->getOption().matches(options::OPT_O)) +OOpt = A->getValue(); dschuff wrote: > This

[PATCH] D70500: [WebAssembly] Enable use of wasm-opt and LTO-enabled system libraries

sunfish updated this revision to Diff 230364. sunfish added a comment. Don't run wasm-opt with -O0. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70500/new/ https://reviews.llvm.org/D70500 Files: clang/lib/Driver/ToolChains/WebAssembly.cpp

[PATCH] D70500: [WebAssembly] Enable use of wasm-opt and LTO-enabled system libraries

sunfish marked an inline comment as done. sunfish added inline comments. Comment at: clang/lib/Driver/ToolChains/WebAssembly.cpp:96 + if (Arg *A = Args.getLastArg(options::OPT_O_Group)) { +if (const char *WasmOptPath = getenv("WASM_OPT")) { + StringRef OOpt = "s";

[PATCH] D70500: [WebAssembly] Enable use of wasm-opt and LTO-enabled system libraries

sunfish updated this revision to Diff 230350. sunfish added a comment. Use PATH instead of WASM_OPT to find wasm-opt. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70500/new/ https://reviews.llvm.org/D70500 Files:

[PATCH] D70500: [WebAssembly] Enable use of wasm-opt and LTO-enabled system libraries

sunfish marked an inline comment as done. sunfish added inline comments. Comment at: clang/lib/Driver/ToolChains/WebAssembly.cpp:96 + if (Arg *A = Args.getLastArg(options::OPT_O_Group)) { +if (const char *WasmOptPath = getenv("WASM_OPT")) { + StringRef OOpt = "s";

[PATCH] D70500: [WebAssembly] Enable use of wasm-opt and LTO-enabled system libraries

sunfish marked an inline comment as done. sunfish added inline comments. Comment at: clang/lib/Driver/ToolChains/WebAssembly.cpp:96 + if (Arg *A = Args.getLastArg(options::OPT_O_Group)) { +if (const char *WasmOptPath = getenv("WASM_OPT")) { + StringRef OOpt = "s";

[PATCH] D70500: [WebAssembly] Enable use of wasm-opt and LTO-enabled system libraries

sunfish updated this revision to Diff 230322. sunfish added a comment. In D70500#1754012 , @sbc100 wrote: > On I just remember why this is probably a bad idea. llvm bitcode is not > designed to be stable, unlike object files, so its probably not a

[PATCH] D70500: [WebAssembly] Enable use of wasm-opt and LTO-enabled system libraries

sunfish marked an inline comment as done. sunfish added inline comments. Comment at: clang/lib/Driver/ToolChains/WebAssembly.cpp:137 + getFilePaths().push_back(getDriver().SysRoot + "/lib/" + MultiarchTriple + + "/llvm-lto"); +}

[PATCH] D70500: [WebAssembly] Enable use of wasm-opt and LTO-enabled system libraries

sunfish marked an inline comment as done. sunfish added inline comments. Comment at: clang/lib/Driver/ToolChains/WebAssembly.cpp:96 + if (Arg *A = Args.getLastArg(options::OPT_O_Group)) { +if (const char *WasmOptPath = getenv("WASM_OPT")) { + StringRef OOpt = "s";

[PATCH] D70500: [WebAssembly] Enable use of wasm-opt and LTO-enabled system libraries

sunfish created this revision. sunfish added reviewers: sbc100, dschuff, aheejin. Herald added subscribers: dexonsmith, steven_wu, hiraditya, jgravelle-google, inglorion, aprantl, mehdi_amini. Herald added a project: clang. When the WASM_OPT environment variable is set, run the wasm-opt tool to

[PATCH] D67739: [WebAssembly] Let users know that wasm64 does not exist

sunfish added a comment. Would it be better to do this check in LLVM, in the backend, with a `report_fatal_error`? Clang for its part already supports what we expect wasm64 will need already, and we have a bunch of tests for it, so it'd be nice to keep that code around and tested. And, doing

[PATCH] D66983: [WebAssembly] Add wasm-specific vector shuffle builtin and intrinsic

sunfish added a comment. In D66983#1651981 , @craig.topper wrote: > DAG combine is supposed to check with TargetLowering::isShuffleMaskLegal. In @tlively's example, it is DAGCombine, and it does check isShuffleMaskLegal. However for wasm, it appears

[PATCH] D66983: [WebAssembly] Add wasm-specific vector shuffle builtin and intrinsic

sunfish added a comment. In D66983#1651977 , @dschuff wrote: > Oh, interesting I didn't notice that those are implementations of the > target-specific intrinsics. I wonder if they do that so they can implement > the intrinsics on hardware that doesn't

[PATCH] D66983: [WebAssembly] Add wasm-specific vector shuffle builtin and intrinsic

sunfish added a comment. Can you say what leads wasm users to maintain such an expectation when, for example, ARM users and x86 users

[PATCH] D66035: [WebAssembly] WIP: Add support for reference types

sunfish added a comment. x86 uses address spaces starting at 256 and counting up for its architecture-specific address spaces. The docs say "Address spaces 1-255 are currently reserved for user-defined code." so we should

[PATCH] D61452: [WebAssembly] Always include /lib in library path

sunfish added a comment. > This allows for us to fall back from arch-specific to generic headers as > needed. The same can be true of libraries. Not all libraries contains > compiled code. `.so` files can also be linker scripts that reference other > libraries in which case they can be

[PATCH] D64900: [WebAssembly] Implement __builtin_wasm_tls_base intrinsic

sunfish added inline comments. Comment at: cfe/trunk/include/clang/Basic/BuiltinsWebAssembly.def:34 TARGET_BUILTIN(__builtin_wasm_tls_size, "z", "nc", "bulk-memory") +TARGET_BUILTIN(__builtin_wasm_tls_base, "v*", "n", "bulk-memory") This can also be "p" for

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

sunfish added a comment. 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` to guarantee that initializes all bytes (with zeros as needed)? `__wasm_init_tls` knows what bytes it's initializing, so we

[PATCH] D63030: [WebAssembly] Modernize include path handling

sunfish accepted this revision. sunfish added a comment. This revision is now accepted and ready to land. LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63030/new/ https://reviews.llvm.org/D63030

[PATCH] D62406: [WebAssembly] Use "linker" as linker shortname.

sunfish accepted this revision. sunfish added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62406/new/ https://reviews.llvm.org/D62406

[PATCH] D61452: [WebAssembly] Always include /lib in library path

sunfish added a comment. If "$sysroot/lib" ends up coming to mean "wasm32" because people come to depend on that, then wasm64 may end up needing to be different in a gratuitous way, which I'd like to avoid. I'd like to keep our sysroots tidy when we can. If some libraries are installed in

[PATCH] D61452: [WebAssembly] Always include /lib in library path

sunfish added a comment. The value of supporting single-arch sysroots is unclear to me. It's always possible to have a sysroot with libraries for just one architecture installed, even with multi-arch paths. Is this just about compatibility with build scripts and tools which are hard-coded to

[PATCH] D61452: [WebAssembly] Always include /lib in library path

sunfish added a comment. If libraries don't correctly install into multi-arch directories, can we fix the libraries? We do this in the wasi-sdk repo to fix up the libc++ and libc++abi libraries, for example. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D61389: Bump DIAG_SIZE_SEMA up to 4000

sunfish abandoned this revision. sunfish added a comment. Cool :) Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61389/new/ https://reviews.llvm.org/D61389 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D61389: Bump DIAG_SIZE_SEMA up to 4000

sunfish created this revision. sunfish added a reviewer: aaron.ballman. Herald added a subscriber: aheejin. Herald added a project: clang. https://reviews.llvm.org/D59520 adds a few new diagnostics, which happens to run into a limit with DIAG_SIZE_SEMA. As suggested here

[PATCH] D61338: [WebAssembly] Use the "wasm32-wasi" triple in tests

sunfish created this revision. sunfish added reviewers: dschuff, sbc100, aheejin. Herald added a subscriber: jgravelle-google. Herald added a project: clang. Similar to https://reviews.llvm.org/D61334, update clang tests to use the "wasm32-wasi" triple, removing the "-musl" environment and

[PATCH] D59520: [WebAssembly] Address review comments on r352930

sunfish updated this revision to Diff 197215. sunfish added a comment. Implemented proper diagnostics for import_name/import_module on functions with definitions, and updated the test. I'm unsure of the `DIAG_SIZE_SEMA` change, but without it, the build fails with this error:

[PATCH] D59520: [WebAssembly] Address review comments on r352930

sunfish marked an inline comment as done. sunfish added inline comments. Comment at: test/Sema/attr-wasm.c:3 + +void name_a() {} + aaron.ballman wrote: > Was this intended to be used somewhere? Probably can just be removed if not. No, you're right that we don't

[PATCH] D59520: [WebAssembly] Address review comments on r352930

sunfish added a comment. In D59520#1434854 , @aaron.ballman wrote: > > Removes errnoneous use of diag::err_alias_is_definition, which turned out > > to be ineffective anyway since functions can be defined later in the > > translation unit and avoid

[PATCH] D59520: [WebAssembly] Address review comments on r352930

sunfish created this revision. sunfish added a reviewer: aaron.ballman. Herald added subscribers: aheejin, jgravelle-google, sbc100, dschuff. Herald added a project: clang. This patch addresses the review comments on r352930: - Removes redundant diagnostic checking code - Removes errnoneous use

[PATCH] D58742: [WebAssembly] Remove uses of ThreadModel

sunfish added a comment. Wasm gives users reasons to want -mthread-model single that other architectures don't, even when -matomics is enabled by default. When shared memory is used, wasm requires modules to declare a max memory size, which is a burden on applications that want to use dynamic

[PATCH] D58742: [WebAssembly] Remove uses of ThreadModel

sunfish added a comment. This is still a little confusing to me. -matomic is supposed to be a subtarget flag, stating that the wasm implementation we will run on supports atomic instructions. -mthread-model posix is about the C++ interpretation -- what style implementation of memory model do

[PATCH] D57874: [WebAssembly] Make thread-related options consistent

sunfish added a comment. > - `-matomics` means `-mthread-model posix` The others sound reasonable, though this one seems a little surprising -- a user might have -matomics enabled because they're targeting a VM that has atomics, but still not want to use -mthread-model posix because their code

[PATCH] D57874: [WebAssembly] Set '-matomics' when '-pthread' is set

sunfish added a comment. That sounds reasonable to me too. Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57874/new/ https://reviews.llvm.org/D57874 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D57577: Make predefined FLT16 macros conditional on support for the type

sunfish added a comment. I think WebAssembly is in the same situation as most other architectures, as discussed here: http://llvm.org/viewvc/llvm-project?view=revision=352221 and should not enable _Float16 yet. The test/Preprocessor/init.c tests were semi-automatically generated, so it wasn't

[PATCH] D57160: [WebAssembly] Add an import_module function attribute

sunfish created this revision. sunfish added reviewers: sbc100, dschuff, aheejin. Herald added subscribers: llvm-commits, jgravelle-google. This adds a C/C++ attribute which corresponds to the LLVM IR wasm-import-module attribute. It allows code to specify an explicit import module.

  1   2   >