[SelectionDAGBuilder] Remove the added base offset in LowerFormalArguments().

[JonPsson1]

LowerCallTo() and LowerArguments() are both providing the PartOffset field for
each split argument part. As these two methods are intended to work together,
it seems ideal that both provide the same offsets. However, currently
LowerArguments() sets the offset from the beginning of a struct, while
LowerCallTo() sets it relative to the first split part.

Removing the PartBase variable in LowerArguments() (and also the SystemZ
workaround recently added) does not seem to break any tests.

Is there a reason to have that around, or would this patch make sense at this point?

@tstellar @arsenm @nikic @momchil-velikov @aeubanks @uweigand

[llvmbot]

@llvm/pr-subscribers-backend-systemz

@llvm/pr-subscribers-llvm-selectiondag

Author: Jonas Paulsson (JonPsson1)

Changes

LowerCallTo() and LowerArguments() are both providing the PartOffset field for
each split argument part. As these two methods are intended to work together,
it seems ideal that both provide the same offsets. However, currently
LowerArguments() sets the offset from the beginning of a struct, while
LowerCallTo() sets it relative to the first split part.

Removing the PartBase variable in LowerArguments() (and also the SystemZ
workaround recently added) does not seem to break any tests.

Is there a reason to have that around, or would this patch make sense at this point?

@tstellar @arsenm @nikic @momchil-velikov @aeubanks @uweigand

---

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

2 Files Affected:

• (modified) llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp (+1-3)
• (modified) llvm/lib/Target/SystemZ/SystemZISelLowering.cpp (+1-8)

diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index 09a0673bfe1bb..4fc06bd4244ed 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -11848,7 +11848,6 @@ void SelectionDAGISel::LowerArguments(const Function &F) {
     SmallVector<Type *, 4> Types;
     ComputeValueTypes(DAG.getDataLayout(), Arg.getType(), Types);
     bool isArgValueUsed = !Arg.use_empty();
-    unsigned PartBase = 0;
     Type *FinalType = Arg.getType();
     if (Arg.hasAttribute(Attribute::ByVal))
       FinalType = Arg.getParamByValType();
@@ -11967,7 +11966,7 @@ void SelectionDAGISel::LowerArguments(const Function &F) {
         // return values.
         ISD::InputArg MyFlags(
             Flags, RegisterVT, VT, ArgTy, isArgValueUsed, ArgNo,
-            PartBase + i * RegisterVT.getStoreSize().getKnownMinValue());
+            i * RegisterVT.getStoreSize().getKnownMinValue());
         if (NumRegs > 1 && i == 0)
           MyFlags.Flags.setSplit();
         // if it isn't first piece, alignment must be 1
@@ -11980,7 +11979,6 @@ void SelectionDAGISel::LowerArguments(const Function &F) {
       }
       if (NeedsRegBlock && Value == NumValues - 1)
         Ins[Ins.size() - 1].Flags.setInConsecutiveRegsLast();
-      PartBase += VT.getStoreSize().getKnownMinValue();
     }
   }
 
diff --git a/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp b/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
index 2511d08a6d0ef..d803bc37f8fc0 100644
--- a/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
+++ b/llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
@@ -2099,17 +2099,10 @@ SDValue SystemZTargetLowering::LowerFormalArguments(
       MVT PartVT;
       unsigned NumParts;
       if (analyzeArgSplit(Ins, ArgLocs, I, PartVT, NumParts)) {
-        // TODO: It is strange that while LowerCallTo() sets the PartOffset
-        // relative to the first split part LowerArguments() sets the offset
-        // from the beginning of the struct. So with {i32, i256}, the
-        // PartOffset for the i256 parts are differently handled. Try to
-        // remove that difference and use PartOffset directly here (instead
-        // of SplitBaseOffs).
-        unsigned SplitBaseOffs = Ins[I].PartOffset;
         for (unsigned PartIdx = 1; PartIdx < NumParts; ++PartIdx) {
           ++I;
           CCValAssign &PartVA = ArgLocs[I];
-          unsigned PartOffset = Ins[I].PartOffset - SplitBaseOffs;
+          unsigned PartOffset = Ins[I].PartOffset;
           SDValue Address = DAG.getNode(ISD::ADD, DL, PtrVT, ArgValue,
                                         DAG.getIntPtrConstant(PartOffset, DL));
           InVals.push_back(DAG.getLoad(PartVA.getValVT(), DL, Chain, Address,

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff origin/main HEAD --extensions cpp -- llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp llvm/lib/Target/SystemZ/SystemZISelLowering.cpp --diff_from_common_commit

⚠️
The reproduction instructions above might return results for more than one PR
in a stack if you are using a stacked PR workflow. You can limit the results by
changing origin/main to the base branch/commit you want to compare against.
⚠️

View the diff from clang-format here.

diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
index 4fc06bd42..cc350a582 100644
--- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
+++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
@@ -11964,9 +11964,9 @@ void SelectionDAGISel::LowerArguments(const Function &F) {
         // For scalable vectors, use the minimum size; individual targets
         // are responsible for handling scalable vector arguments and
         // return values.
-        ISD::InputArg MyFlags(
-            Flags, RegisterVT, VT, ArgTy, isArgValueUsed, ArgNo,
-            i * RegisterVT.getStoreSize().getKnownMinValue());
+        ISD::InputArg MyFlags(Flags, RegisterVT, VT, ArgTy, isArgValueUsed,
+                              ArgNo,
+                              i * RegisterVT.getStoreSize().getKnownMinValue());
         if (NumRegs > 1 && i == 0)
           MyFlags.Flags.setSplit();
         // if it isn't first piece, alignment must be 1

[JonPsson1]

@tstellar Any comment on this (you added this a long time ago)

[JonPsson1]

I guess I will commit this soon then if there are no further comments... @uweigand

[tstellar]

No objections to this.