[PATCH] D125680: Correctly legalise stackmap operands

[Edd Barrett via Phabricator via llvm-commits]

vext01 added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:5527-5528
+    } else {
+      // FIXME: https://github.com/llvm/llvm-project/issues/55609
+      return Optional<SDValue>();
+    }
----------------
arsenm wrote:
> Just leave the node as-is? It will fail to legalize later
Sorry, there was some confusion at my end.

Leaving the node as-is (i.e. `return SDValue(N, 0);`) is the case that will cause an infinite loop in the legaliser. Returning a "null" value (i.e. `return SDValue()`) is fine though.

If I put a print at the top of this function:
```
errs() << __func__ << ": " << OpNo << ": "; N->dump();
```

And update the `return SDValue()` to `return SDValue(N, 0);`, and then use an input that covers one of those cases, e.g.:

```
declare void @llvm.experimental.stackmap(i64, i32, ...)

define void @f() {
  call void (i64, i32, ...) @llvm.experimental.stackmap(i64 0, i32 0, i128 9223372036854775808)
  ret void
}
```

Then we see the function called indefinitely, trying to legalise the same operand forever:
```
ExpandIntOp_STACKMAP: 4: t8: ch,glue = stackmap t3, t3:1, TargetConstant:i64<0>, TargetConstant:i32<0>, Constant:i128<9223372036854775808>
ExpandIntOp_STACKMAP: 4: t8: ch,glue = stackmap t3, t3:1, TargetConstant:i64<0>, TargetConstant:i32<0>, Constant:i128<9223372036854775808>
ExpandIntOp_STACKMAP: 4: t8: ch,glue = stackmap t3, t3:1, TargetConstant:i64<0>, TargetConstant:i32<0>, Constant:i128<9223372036854775808>
ExpandIntOp_STACKMAP: 4: t8: ch,glue = stackmap t3, t3:1, TargetConstant:i64<0>, TargetConstant:i32<0>, Constant:i128<9223372036854775808>
ExpandIntOp_STACKMAP: 4: t8: ch,glue = stackmap t3, t3:1, TargetConstant:i64<0>, TargetConstant:i32<0>, Constant:i128<9223372036854775808>
ExpandIntOp_STACKMAP: 4: t8: ch,glue = stackmap t3, t3:1, TargetConstant:i64<0>, TargetConstant:i32<0>, Constant:i128<9223372036854775808>
ExpandIntOp_STACKMAP: 4: t8: ch,glue = stackmap t3, t3:1, TargetConstant:i64<0>, TargetConstant:i32<0>, Constant:i128<9223372036854775808>
ExpandIntOp_STACKMAP: 4: t8: ch,glue = stackmap t3, t3:1, TargetConstant:i64<0>, TargetConstant:i32<0>, Constant:i128<9223372036854775808>
ExpandIntOp_STACKMAP: 4: t8: ch,glue = stackmap t3, t3:1, TargetConstant:i64<0>, TargetConstant:i32<0>, Constant:i128<9223372036854775808>
...
```

This is due to the special casing in `DAGTypeLegalizer::ExpandIntegerOperand()`:
```
Res = ExpandIntOp_STACKMAP(N, OpNo);
...
    // If the result is null, the sub-method took care of registering results etc.                                                                                                                                                            
    if (!Res.getNode()) return false;                                                                                                                                                                                                         
                                                                                                                                                                                                                                              
    // If the result is N, the sub-method updated N in place.  Tell the legalizer                                                                                                                                                             
    // core about this.                                                                                                                                                                                                                       
    if (Res.getNode() == N)                                                                                                                                                                                                                   
      return true;      
```

Returning `SDValue()` hits the `return false`, whereas `return SDValue(N, 0)` hits `return true`.

The return value is whether or not the operand needs another round of legalisation. At the call site of `ExpandIntegerOperand()`, there is:

```
NeedsReanalyzing = ExpandIntegerOperand(N, i);
```

So we need to return `false` to:
 - not loop forever
 - bail out with the right error.

So I believe `return SDValue()` to be correct. The latest diff reflects this.


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

https://reviews.llvm.org/D125680