[SelectionDAG][PPC][SystemZ] Fix GET_DYNAMIC_AREA_OFFSET chain result

[s-barannikov]

The node has a chain, but it wasn't handled correctly:

• DAG builder didn't update the root
• DAG legalizer replaced the chain result with integer 0
• PPC and SystemZ lowerings didn't return the chain result at all

SystemZ lowering is still incorrect because it now returns the source chain. Fixing it turned out to be a non-trivial task, so I left a FIXME.

[llvmbot]

@llvm/pr-subscribers-backend-powerpc

@llvm/pr-subscribers-backend-systemz

Author: Sergei Barannikov (s-barannikov)

Changes

The node has a chain, but it wasn't handled correctly:

• DAG builder didn't update the root
• DAG legalizer replaced the chain result with integer 0
• PPC and SystemZ lowerings didn't return the chain result at all

SystemZ lowering is still incorrect because it now returns the source chain. Fixing it turned out to be a non-trivial task, so I left a FIXME.

---

Full diff: https://github.com/llvm/llvm-project/pull/116507.diff

4 Files Affected:

• (modified) llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp (+1-1)
• (modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp (+3-3)
• (modified) llvm/lib/Target/PowerPC/PPCISelLowering.cpp (+1-5)
• (modified) llvm/lib/Target/SystemZ/SystemZISelLowering.cpp (+4-1)

diff --git a/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
index 1480bd98c685e1..1748cfb215887c 100644
--- a/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
@@ -3499,7 +3499,7 @@ bool SelectionDAGLegalize::ExpandNode(SDNode *Node) {
     break;
   case ISD::GET_DYNAMIC_AREA_OFFSET:
     Results.push_back(DAG.getConstant(0, dl, Node->getValueType(0)));
-    Results.push_back(Results[0].getValue(0));
+    Results.push_back(Node->getOperand(0));
     break;
   case ISD::FCOPYSIGN:
     Results.push_back(ExpandFCOPYSIGN(Node));
diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index 9d729d448502d8..fd291f69a9f773 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -7401,9 +7401,9 @@ void SelectionDAGBuilder::visitIntrinsicCall(const CallInst &I,
     if (PtrTy.getFixedSizeInBits() < ResTy.getFixedSizeInBits())
       report_fatal_error("Wrong result type for @llvm.get.dynamic.area.offset"
                          " intrinsic!");
-    Res = DAG.getNode(ISD::GET_DYNAMIC_AREA_OFFSET, sdl, DAG.getVTList(ResTy),
-                      Op);
-    DAG.setRoot(Op);
+    Res =
+        DAG.getNode(ISD::GET_DYNAMIC_AREA_OFFSET, sdl, {ResTy, MVT::Other}, Op);
+    DAG.setRoot(Res.getValue(1));
     setValue(&I, Res);
     return;
   }
diff --git a/llvm/lib/Target/PowerPC/PPCISelLowering.cpp b/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
index daddd064b0a8fd..362746c6457017 100644
--- a/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
+++ b/llvm/lib/Target/PowerPC/PPCISelLowering.cpp
@@ -7916,16 +7916,12 @@ PPCTargetLowering::LowerGET_DYNAMIC_AREA_OFFSET(SDValue Op,
                                                 SelectionDAG &DAG) const {
   SDLoc dl(Op);
 
-  // Get the correct type for integers.
-  EVT IntVT = Op.getValueType();
-
   // Get the inputs.
   SDValue Chain = Op.getOperand(0);
   SDValue FPSIdx = getFramePointerFrameIndex(DAG);
   // Build a DYNAREAOFFSET node.
   SDValue Ops[2] = {Chain, FPSIdx};
-  SDVTList VTs = DAG.getVTList(IntVT);
-  return DAG.getNode(PPCISD::DYNAREAOFFSET, dl, VTs, Ops);
+  return DAG.getNode(PPCISD::DYNAREAOFFSET, dl, Op->getVTList(), Ops);
 }
 
 SDValue PPCTargetLowering::LowerSTACKRESTORE(SDValue Op,
diff --git a/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp b/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
index 78d91299a357dd..ee34c78d8a924a 100644
--- a/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
+++ b/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
@@ -4127,7 +4127,10 @@ SDValue SystemZTargetLowering::lowerGET_DYNAMIC_AREA_OFFSET(
     SDValue Op, SelectionDAG &DAG) const {
   SDLoc DL(Op);
 
-  return DAG.getNode(SystemZISD::ADJDYNALLOC, DL, MVT::i64);
+  // FIXME: SystemZISD::ADJDYNALLOC should be chained.
+  SDValue Result = DAG.getNode(SystemZISD::ADJDYNALLOC, DL, MVT::i64);
+  SDValue Chain = Op->getOperand(0);
+  return DAG.getMergeValues({Result, Chain}, DL);
 }
 
 SDValue SystemZTargetLowering::lowerSMUL_LOHI(SDValue Op,

[s-barannikov]

No tests. The issue was discovered when working on a SDNode verification feature, I'll present an RFC later.

[uweigand]

I don't quite see what this chain is intended to accomplish. At least on SystemZ, GET_DYNAMIC_AREA_OFFSET returns a value that is constant everywhere within the given function (it can differ between functions). So there is no reason to tie its evaluation to any particular place (as enforced by a chain), and the back-end currently optimizes resolution of ADJDYNALLOC based on that property.

I believe the same should be true for PowerPC as well, although they may not optimize GET_DYNAMIC_AREA_OFFSET as much as we do.

[s-barannikov]

I got confused by the wording "offset to the most recent dynamic alloca". On my target stack grows up, and the offset is therefore a dynamic value (equal to the size of the most recent allocation). However, LangRef says:

For targets where stack grows upwards, the situation is a bit more complicated, because subtracting this value from stack pointer would get the address one past the end of the most recent dynamic alloca.

So this "offset" should not be read as "offset to the beginning".

I abandoning this patch then.