[PATCH] D54614: [WebAssembly] Fix MCNullStreamer support

Eugene Sharygin via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Nov 16 23:31:06 PST 2018


eush added a comment.

In https://reviews.llvm.org/D54614#1301891, @aheejin wrote:

> I opened https://reviews.llvm.org/D54660. I think this way we don't need to use `if (WebAssemblyTargetStreamer *TS = getTargetStreamer()) ...` every time we use `getTargetStreamer()`. What do you think?


I think that's great! I didn't know about `RegisterNullTargetStreamer` - it looks to me like the correct way to support MCNullStreamer.



================
Comment at: test/CodeGen/WebAssembly/null-streamer.ll:11
+define i32 @f(i8* %p)  {
+  call void @llvm.wasm.throw(i32 0, i8* %p)
+  call void @g()
----------------
aheejin wrote:
> sbc100 wrote:
> > eush wrote:
> > > sbc100 wrote:
> > > > Can you simplify this test, drop the trow and remove the exception arguments from llc?
> > > Can I wonder why? A simpler test would only exercise changes in EmitFunctionBodyStart and wouldn't need any changes in EmitEndOfAsmFile.
> > Oh I see.   I just found a it a little strange to have those options enabled, but I suppose it makes sense.  Alternatively you could call alloca in order to trigger the use of __stack_pointer.
> > 
> > Also, if you want to be sure you excersise those parts maybe you would want to at a CHECK: for the .globaltype and/or .eventtype at the end?
> This is nullstreamer, so I guess it wouldn't print anything so we can't check.
> This is nullstreamer, so I guess it wouldn't print anything so we can't check.

That is unless we add a second RUN line with a default streamer, just for the checks.


Repository:
  rL LLVM

https://reviews.llvm.org/D54614





More information about the llvm-commits mailing list