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

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:1984
+                       TargetSpecificAttr<TargetWebAssembly> {
+  let Spellings = [Clang<"wasm_async">];
+  let Documentation = [WebAssemblyAsyncDocs];
----------------
brendandahl wrote:
> sbc100 wrote:
> > Should we call this em_async or emscripten_async since this is an emscripten-specific attribute?
> I wouldn't say this is emscripten specific. You could use this with clang+binaryen without emscripten.
Maybe we can at least say that its web-specific?  Would it make any sense for non-web platforms?

If you are using clang + binaryen + web its hard to imagine a non-emscripten triple being used.. but maybe?


================
Comment at: lld/test/wasm/async.ll:18
+    call void @bar()
+    ret void
+}
----------------
For lld tests I've been trying for a while now to move away from `.ll` and just use `.s` format. 

The only reason we have any `.ll` files at all in this directory is because for a long time we didn't have a working `.s` format.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:561
+void WebAssemblyAsmPrinter::EmitAsync(Module &M) {
+  SmallVector<MCSymbol *, 4> EmittedAsyncs;
+
----------------
Perhaps call this `AsyncFuncs` or `AsyncFuncNames`?


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyAsmPrinter.cpp:567
+      EmittedAsyncs.push_back(Sym);
+    }
+  }
----------------
No need for curlys around one-liners in llvm


================
Comment at: llvm/test/MC/WebAssembly/async.s:20
+# CHECK-OBJ-NEXT:        Index:           0
+# CHECK-OBJ-NEXT:        Offset:          0x0
+# CHECK-OBJ-NEXT:     Name:            async
----------------
Indentation look wrong here.. `Index`, `Offset` and `Name` should align with `Type` I think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150803