[WebAssembly] Fix reftype load/store match with idx from call

[pmatos]

Summary

Implement support for matching an index from a WebAssembly CALL
instruction. Add test.

[sbc100]

llvm/test/CodeGen/WebAssembly/externref-tableset.ll

82

Is this returning the ID of a table, or the ID of slot within a table? I seem like its the latter, in which case maybe its name is a little confusing? How about calling this get_table_slot ?

[pmatos]

I think this needs to be more generic, rather than allowing just calls... as I found a similar problem when the node is a CopyFromReg. I will refactor this patch.

llvm/test/CodeGen/WebAssembly/externref-tableset.ll

82

You're right, it's the latter. I will rename.

[pmatos]

Make the fix slightly more generic. I have found while writing LLVM IR with lots of usage of store and loads on reference types and intrinsics, that there are several cases where the Idx to the table can be the result of a call or copy from reg, etc. These have 2 values, the main value and the chain so checking if the Idx has a single value is the wrong approach. This seems to work as by default we use the first value of the Node and we don't need to impose any restrictions on it. The only case this could possibly fail is if we get a Node whose value we want to use is not the first value but I cannot imagine a case where that could happen and I didn't come accross it.

[pmatos]

Rebase on main and solve conflict with D115511, which just landed.

[tlively]

Code LGTM, but I don't quite understand what the bug was, so I'm not sure about the test coverage.

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp

1493–1494

Does the test in this revision cover this Idx.isUndef() case? I would have expected to see an undef in the IR.

[pmatos]

In D115327#3193154, @tlively wrote:

Code LGTM, but I don't quite understand what the bug was, so I'm not sure about the test coverage.

I should have definitely explained this better, I am sorry.

In the C++ code matching a LowerStore or LowerLoad from a table global into TABLE_GET or TABLE_SET, we used to check that the Idx variable for the table access, i.e. the indice into the table has only one value (checks for Idx->getNumValues() != 1 and doesn't lower it if true). In one of the cases I am translating from the milestones work (https://github.com/Igalia/ref-cpp/blob/13537129e16a48fc62ad719f63fe592d11ae6d2f/milestones/m3/test.ll#L137) the index of the table comes from a call to a function. The call however doesn't have a single value though, it has two. One for the return value and one for the chain and therefore LLVM was not lowering the node and instead it was failing during selection. I didn't do anything special besides removing the check for single-valuedness from Idx, which works. I am not 100% sure it always works though, so if you can think of another time when this wouldn't work, it would be great to know. For example, the way it currently works is that if the node has more than one value, then the first value is used. Since chains are the last value, this works for calls but it would fall apart if there's a node for which the value that contains the indice into the table is not in first value position. I can't however think of a situation where this would happen.

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp

1493–1494

Yes, good point. I should write a test covering this case.

[pmatos]

Remove isUndef check from code. I cannot find a codepath that generates an undef Idx at this point. All undef values end up being propogated down to the respective GEP. So simplifying and removing check.

[pmatos]

Rebased on top of main. All seems good.

@tlively any further comments?

[tlively]

An example situation where there might be more than one value is in a multivalue call, but Idx should only refer to whichever value is actually used as the offset and I agree that it shouldn't matter whether there are other values in the same node or not.

[tlively]

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp

1493–1494

Should this just be an assertion? I would be surprised if a GlobalAddressSDNode could ever have more than one value.

llvm/lib/Target/WebAssembly/WebAssemblyISelLowering.cpp

bool WebAssemblyTargetLowering::MatchTableForLowering(SelectionDAG &DAG,
   //
   if (IsWebAssemblyTable(Base)) {
     GA = cast<GlobalAddressSDNode>(Base);
     Idx = DAG.getConstant(0, DL, MVT::i32);
   } else {
     GA = dyn_cast<GlobalAddressSDNode>(Base->getOperand(0));
     if (GA) {
       // We are in Case 2 above.
       Idx = Base->getOperand(1);
-      if (!Idx || GA->getNumValues() != 1 || Idx->getNumValues() != 1)
-        return false;
+      assert(GA->getNumValues() == 1);
     } else {
       // This might be Case 1 above (or an error)
       SDValue V = Base->getOperand(0);
       GA = dyn_cast<GlobalAddressSDNode>(V->getOperand(1));
 
       if (V->getOpcode() != ISD::ADD || V->getNumOperands() != 2 || !GA)
         return false;
 

llvm/test/CodeGen/WebAssembly/externref-tableset.ll

 ; CHECK-NEXT:  end_function
   %j = call i32 @set_offset()
   %off = add nsw i32 %i, %j
   %p = getelementptr [0 x %externref], [0 x %externref] addrspace (1)* @externref_table, i32 0, i32 %off
   store %externref %g, %externref addrspace(1)* %p
   ret void
 }
 
+declare i32 @get_table_slot() local_unnamed_addr
+
+define void @set_externref_table_with_id_from_call(%externref %g) {
+; CHECK-LABEL: set_externref_table_with_id_from_call:
+; CHECK-NEXT:  .functype       set_externref_table_with_id_from_call (externref) -> ()
+; CHECK-NEXT:  call    get_table_slot
+; CHECK-NEXT:  local.get       0
+; CHECK-NEXT:  table.set       externref_table
+; CHECK-NEXT:  end_function
+  %id = call i32 @get_table_slot()
+  %p = getelementptr [0 x %externref], [0 x %externref] addrspace (1)* @externref_table, i32 0, i32 %id
+  store %externref %g, %externref addrspace(1)* %p
+  ret void
+}
+
 ; CHECK: .tabletype externref_table, externref