[PATCH] D149917: [lld][WebAssembly] Add --keep-section flag

[Alon Zakai via Phabricator via cfe-commits]

kripken requested changes to this revision.
kripken added inline comments.
This revision now requires changes to proceed.



Comment at: clang/lib/Driver/ToolChains/WebAssembly.cpp:154
+  if (!WasmOptPath.empty()) {
+CmdArgs.push_back("--keep-secton=target_features");
+  }

Shouldn't `secton` => `section`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149917

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alon Zakai via Phabricator via cfe-commits]

kripken added inline comments.



Comment at: clang/lib/Basic/Targets/WebAssembly.h:175
+if (T.isOSEmscripten())
+  resetDataLayout("e-m:e-p:32:32-i64:64-f128:64-n32:64-S128-ni:1");
+else

Should this not be

resetDataLayout("e-m:e-p:64:64-i64:64-f128:64-n32:64-S128-ni:1");

(that is, the first two numbers should be 64?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D105749

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alon Zakai via Phabricator via cfe-commits]

kripken accepted this revision.
kripken added a comment.
This revision is now accepted and ready to land.

I believe we have all agreed that this is the right path forward, and so this 
can land?

Doing this change now in emscripten does not prevent us from changing the 
emscripten ABI again later, as the ABI is unstable (which is why we can make 
this change itself). We definitely should in the future consider unifying the 
emscripten triple with WASI, but for now, this change will be a strict 
improvement in emscripten.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104808

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alon Zakai via Phabricator via cfe-commits]

kripken added a comment.

In D104808#2836942 , @sunfish wrote:

> 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.

I definitely think we should unify them as much as possible. Ideally we would 
all make this change.

The motivation for the change is that right now we are losing either some 
correctness or some performance. Either is a burden, and a possible future 
float128 in wasm doesn't seem strong enough of a justification to me - should 
wasm add float128, we can consider a new ABI at that point. Does that sound 
reasonable?

> 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 from being able to 
> call `malloc` and get a buffer aligned for SIMD. Do we have more information 
> on how much this matters in practice?

I think the discussions converged on it being ok with the spec, but perhaps a 
problem in practice, but portable SIMD code seems smart enough to avoid the 
issue. I don't think I've seen a bug report mentioning SIMD, but I may have 
missed one.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104808

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D102791: [WebAssembly] Warn on exception spec for Emscripten EH

[Alon Zakai via Phabricator via cfe-commits]

kripken accepted this revision.
kripken added a comment.
This revision is now accepted and ready to land.

I'm not very familiar with this code, but it looks right, and sgtm to add a 
warning here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102791

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alon Zakai via Phabricator via cfe-commits]

kripken added a comment.

In D77908#1977039 , @sbc100 wrote:

> As a less controversial version of this change I could instead create a new 
> CPU called `current` and leave `generic` as is (basically leave it at mvp) 
> until we can agree that a features is widespread enough to warrant being part 
> of generic?


That makes a lot of sense to me. `current` or `current-spec` or such seems 
pretty clear. Then `generic` stays as it always was (at mvp) and that seems 
safer for the ecosystem.

Btw, how does LLVM handle this issue with other backends? When say Intel 
releases a new CPU with a new feature, are those automatically applied in the 
`generic` CPU for Intel? (and hence people that want older CPUs will get 
breakage unless they change the CPU  target) If those are automatically 
applied, at what frequency/policy?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77908



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alon Zakai via Phabricator via cfe-commits]

kripken added a comment.

Is the general plan for LLVM documented somewhere?

It's not obvious to me why something being in the wasm spec means it should be 
enabled by default in LLVM. (It's also not obvious to me that is wrong! I'm 
just not sure what the reasoning is here.)

In particular, something being in the wasm spec doesn't mean it is widespread 
yet. I see https://github.com/emscripten-core/emscripten/pull/10885 proposes to 
diverge the emscripten defaults from LLVMs. That seems odd to me - why 
shouldn't those be the same? If they aren't the same it's a potential source of 
confusion in multiple ways. For example, if the reasoning is "LLVM moves with 
the spec, emscripten moves with the web" then that means *non*-emscripten 
toolchains targeting the web have more work to do. Also comparisons between 
toolchains will get harder to get apples-to-apples.

Again, not opposed to this, just not sure what the big picture is.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77908



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alon Zakai via Phabricator via cfe-commits]

kripken accepted this revision.
kripken added inline comments.
This revision is now accepted and ready to land.



Comment at: clang/include/clang/Basic/AttrDocs.td:4173
+attribute for the WebAssembly target. This attribute may be attached to a
+function declaration, where it causes the be exported from the linked
+WebAssembly module.

"the be" => "the function to be"


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76547



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D40526: [WebAssembly] Change size_t to `unsigned long`

[Alon Zakai via Phabricator via cfe-commits]

azakai added subscribers: sunfish, jgravelle-google.
azakai added a comment.

This has also shown up in a game engine middleware codebase, so it may be a
broader issue - people seem to assume size_t is one of the int*_t types.


Repository:
  rC Clang

https://reviews.llvm.org/D40526



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits