[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:4675-4676
+      Res = MaybeRes.getValue();
+    else
+      goto fail;
+    break;
----------------
arsenm wrote:
> Don't see why you are introducing this goto
It's because it's the only path to the error state contained in the `default` branch of the switch.

In an earlier conversation we said that we wanted to trigger the default "can't legalise this" error if we encounter a case we cannot handle.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:5507
+
+Optional<SDValue> DAGTypeLegalizer::ExpandIntOp_STACKMAP(SDNode *N,
+                                                         unsigned OpNo) {
----------------
arsenm wrote:
> Don't see why you're using Optional. Just use SDValue() like everything else?
If we return `SDValue()` then we are signalling that the handler has already replaced the node with a new node with the operand in question legalised.

If we do that, then the legaliser will get stuck in an infinite loop trying to legalise the operand that we cannot handle, since we never actually replace the operand.

I introduced the optional so that we could signal the error state and break out of the legaliser loop (i.e. the goto you questioned).


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

https://reviews.llvm.org/D125680