[PATCH] D54873: [WebAssembly] Make signature index not a part of EventType

[Sam Clegg via Phabricator via llvm-commits]

sbc100 added inline comments.


================
Comment at: include/llvm/BinaryFormat/Wasm.h:88
   uint32_t Attribute;
-  uint32_t SigIndex;
 };
----------------
aheejin wrote:
> sbc100 wrote:
> > My initial reaction is that we should leave this as is.
> > 
> > The linker shouldn't be modifying the input EventTypes but instead creating a new set of output eventtypes based on the input ones.  At least that how it makes sense to me.  I will take a look at the lld side change and see if I can make some suggestions there.
> You mean we should create separate set of everything, like `WasmEventType`, `WasmGlobalType`, ... and copy all data from the old structures to the new structures in the linker to resolve this? Wouldn't that be too much duplication? We already have separate `WasmEventType` and `WasmGlobalType`, ... in `WasmObjectWriter` side (lib/MC). I would want to avoid adding another set of all these, just to resolve signature index. And if we only add `WasmEventType` for the output type in the linker, that would look inconsistent as well.
> 
> And the original reason `SigIndex` was a part of `WasmEventType` was just the [spec](https://github.com/WebAssembly/exception-handling/blob/master/proposals/Exceptions.md#event_type) has a table structured that way, which is not very important reason. (This CL does not change the binary spec itself)
> 
> And the other thing is, taking `SigIndex` out of a certain type (here `WasmEventType`) might be more extensible way, for when we have multi-value globals. In that case every type can have an optional `SigIndex` seems a easier way to handle things.
I think so yes.  If you look at the InputGlobal class in lld today it looks like already makes copy of the InputGlobal.   I imagine the InputEvent class would have the WasmEvent in it.  These object do not need to be the exact objects that get written to the output.  It happens that for InputGlobal we do end up calling writeGlobal with that as the input, but for events you could do something like this:

e.g.
```
for (const InputEvent *E : InputEvent)  {
  WasmEvent NewEv = *E->Event;
  // Replace the input sig index with the output sig index.
  NewEv->Type->SigIndex = lookupType(E->Type>SigIndex);
   writeEvent(OS, NewEv); 
}
```


================
Comment at: include/llvm/BinaryFormat/Wasm.h:92
 struct WasmEvent {
   uint32_t Index;
   WasmEventType Type;
----------------
I also think it nice if you can get the signature from the WasmEvent stuct.    Rather than separating the events from their signatures.  Seems to fit better with the binary representation.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54873