[PATCH] D59343: [WebAssembly] Use passive segments when memory is shared

Thu Mar 14 14:47:45 PDT 2019

tlively marked an inline comment as done.
tlively added inline comments.

================
Comment at: lld/test/wasm/data-segment-merging.ll:12
 ; RUN: llc -filetype=obj %s -o %t.data-segment-merging.o
+; RUN: llc -filetype=obj %s -o %t.data-segment-merging.atomics.o -mattr=+atomics

----------------
sbc100 wrote:
> tlively wrote:
> > sbc100 wrote:
> > > The code looks like passive initiazlition is driver by the "--shared-memory" flag rather than atomics.   Wouldn't it make more sense to use "SHARED" over "ATOMICS" in this test?
> > D59284 makes the "atomics" feature imply the use of shared memory, and in this test the flag that causes shared memory to be used is `-mattr=+atomics`, so I think the current naming accurately describes what is happening. I expect the common way to get shared memory is by using the atomics feature, so perhaps we can even deprecate the --shared-memory flag, WDYT?
> I'm not sure.   Part of me likes the explicit requirment/ability to pass this kind of thing as a linker flag.   I think shared vs non-shared memory is significant enough that I'm not sure it should be driven my object file input. 
> 
> e.g. As a developer, I don't necessarily want the accidental inclusion of the bad object file to change the type of output object.  It would be like infering `-shared` if I pass objects build with `-PIC`.     I think I'd rather get a linker error (object files use atomics, did you mean to pass --shared-memory?)
That's a reasonable argument. This CL introduces a similar error when atomics are used but a max memory size is not supplied. But I think requiring an explicit `--shared-memory flag` when the linker can infer that shared memory is necessary is an unnecessary speed bump for users. Inferring shared memory is consistent with our intent in the clang flags that using the atomics feature implies multithreading. The more robust mechanism for guarding against bad object files is explicitly specifying features with `--features=...`.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59343/new/

https://reviews.llvm.org/D59343