[PATCH] D95425: Implementation of global.get/set for reftypes in LLVM IR

[Andy Wingo via Phabricator via cfe-commits]

wingo added inline comments.


================
Comment at: llvm/lib/Target/WebAssembly/WebAssemblyInstrTable.td:15
 multiclass TABLE<WebAssemblyRegClass rt> {
+  let mayLoad = 1 in
   defm TABLE_GET_#rt : I<(outs rt:$res), (ins table32_op:$table),
----------------
pmatos wrote:
> tlively wrote:
> > wingo wrote:
> > > I think you may need `hasSideEffects = 0` for these annotations to have an effect.
> > I would be surprised if this were true!
> Why would this be the case? If I remember correctly, I added `mayLoad` and `mayStore` here so that lowering includes a chain. And this works without the need for `hasSideEffects`. Unless you think this is required for other reaons, but `mayLoad` works with it.
Yeah I misunderstood, I was thinking that `mayLoad` and `mayStore` were a subset of the side effect annotations of `hasSideEffects`, which defaults to 1 effectively (grep for `guessInstructionProperties`).  But no, the side effects modelled here are the disjoint bits `mayLoad`, `mayStore`, and `hasSideEffects`; `mayRaiseFPException` would appear to be a subset of `hasSideEffects`.  Still, may best to explicitly set `hasSideEffects` to 1 for `table.get ` and `table.set` just as documentation that they can trap if the index is out-of-bounds.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95425