[PPC][AIX] Fix toc-data peephole bug and some related cleanup.

[sfertile]

Summary

Set the ReplaceFlags variable to false, since there is code meant only for the ADDItocHi/ADDItocL nodes guarded by this. This has the side effect of disabling the peephole on AIX when the load/store instruction has a non-zero offset fixing the bug reported in https://github.com/llvm/llvm-project/issues/63927. This patch also fixes retrieving the ImmOpnd node from the AIX small code model pseudos and does the same for the register operand node. This allows cleaning up the later calls to replaceOperands. Finally move calculating the MaxOffset into the code guarded by ReplaceFlags as it is only used there and the comment is specific to the ELF ABI.

[hubert.reinterpretcast]

@amyk, fyi, since it appears you have plans to change code in the same vicinity for https://reviews.llvm.org/D155600 and its follow-up (for non-zero load/store offsets).

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp

7669–7670

This is moved a bit too far from where UpdateHBase is being set (thus making the initialization of HBase here confusing).

Suggestion:

• Use the default value for HBase.
• Move these declarations down to immediately before the if (ReplaceFlags) block.
• Move the RegOperand declaration to immediately before that of ImmOpnd.
• Use RegOperand in place of HBase until after the ADDIStocHA8 check.
• Assign to HBase the value of RegOperand.

7670

ImmOpnd and RegOperand should never be associated with the same Base operand index. The conditions should be harmonized. It seems to me that the OpcodeIsAIXTocData condition is the more conservative one.

7728–7729

This comment is incorrect. Substitution does not always occur with the operands reversed now.

[hubert.reinterpretcast]

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp

7752

Can we add a FIXME to verify and better document the reason why Offset needs to be a multiple of 4 when the alignment is less than 4? It is not about the encoding of the instruction: the value of Offset comes directly from the original load/store instruction on the path that reaches this check.

[sfertile]

Address first round of review comments.

[hubert.reinterpretcast]

LGTM with minor comments. Thanks for the patch!

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp

7656

Suggest rename to OpcodeIsAIXSmallTocData.

7663–7664

Minor nit: Be consistent on whether to call getOperand through the SDValue or on the SDNode directly.

7670–7674

Move variables meant to be set by the block to immediately before the block.

[nemanjai]

It seems to me that this section of this function has built up a significant amount of technical debt and we should really try to get it refactored so it is clearer what is happening and why. Please note that this is a statement about the existing implementation, not just about the updates in this patch.

Just some examples:

• There is a statement that these pseudo instructions are used exclusively for AIX small code model for globals defined in the TOC. Is this enforced somehow? Will it become a problem if it is used in a different context?
• The operands for these instructions are reversed. Why? If we have defined something specifically for this particular optimization and decided to define it in a way that is deliberately different from other similar artifacts, that seems misguided to me.
• It seems that the ReplaceFlags variable controls more than whether or not we're replacing the flags
• It should probably be clarified what this code does in different contexts (ELFv1, ELFv2, AIX, different code models - particularly pointing out the differences)

Also, we committed this as well as backported it to the release branch. This is a non-trivial patch that didn't really have time in trunk to really bake in. I think a more appropriate course of action would probably have been to turn off the optimization due to the bug, get that ported back and then work on fixing and re-enabling it in trunk in the early part of the release.

llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp

while (Position != CurDAG->allnodes_begin()) {
     case PPC::ADDItlsldL:
       Flags = PPCII::MO_TLSLD_LO;
       break;
     case PPC::ADDItocL:
       Flags = PPCII::MO_TOC_LO;
       break;
     case PPC::ADDItoc:
     case PPC::ADDItoc8:
+      ReplaceFlags = false;
       if (RequiresMod4Offset) {
         if (GlobalAddressSDNode *GA =
                 dyn_cast<GlobalAddressSDNode>(Base.getOperand(0))) {
           const GlobalValue *GV = GA->getGlobal();
           Align Alignment = GV->getPointerAlignment(CurDAG->getDataLayout());
           // XMC_TD global that is underaligned being accessed with a DS form
           // instruction.
           if (Alignment < 4)
             continue;
         }
       }
       break;
     }
 
-    SDValue ImmOpnd = Base.getOperand(1);
+    const unsigned BaseOpcode = Base.getMachineOpcode();
+    // ADDItoc and ADDItoc8 are pseudos used exclusively by AIX small code
+    // model when a global is defined in the TOC.
+    const bool OpcodeIsAIXSmallTocData =
+        BaseOpcode == PPC::ADDItoc || BaseOpcode == PPC::ADDItoc8;
+
+    SDValue RegOperand;
+    SDValue ImmOpnd;
+    // The AIX small code model nodes have the operands reversed.
+    if (OpcodeIsAIXSmallTocData) {
+      RegOperand = Base.getOperand(1);
+      ImmOpnd = Base.getOperand(0);
+    } else {
+      RegOperand = Base.getOperand(0);
+      ImmOpnd = Base.getOperand(1);
+    }
+
+    int Offset = N->getConstantOperandVal(FirstOp);
 
+    SDValue HBase;
+    bool UpdateHBase = false;
+    if (ReplaceFlags) {
       // On PPC64, the TOC base pointer is guaranteed by the ABI only to have
-    // 8-byte alignment, and so we can only use offsets less than 8 (otherwise,
-    // we might have needed different @ha relocation values for the offset
-    // pointers).
+      // 8-byte alignment, and so we can only use offsets less than 8
+      // (otherwise, we might have needed different @ha relocation values for
+      // the offset pointers).
       int MaxDisplacement = 7;
       if (GlobalAddressSDNode *GA = dyn_cast<GlobalAddressSDNode>(ImmOpnd)) {
         const GlobalValue *GV = GA->getGlobal();
         Align Alignment = GV->getPointerAlignment(CurDAG->getDataLayout());
         MaxDisplacement = std::min((int)Alignment.value() - 1, MaxDisplacement);
       }
 
-    bool UpdateHBase = false;
-    SDValue HBase = Base.getOperand(0);
-
-    int Offset = N->getConstantOperandVal(FirstOp);
-    if (ReplaceFlags) {
       if (Offset < 0 || Offset > MaxDisplacement) {
         // If we have a addi(toc@l)/addis(toc@ha) pair, and the addis has only
         // one use, then we can do this for any offset, we just need to also
         // update the offset (i.e. the symbol addend) on the addis also.
-        if (Base.getMachineOpcode() != PPC::ADDItocL)
+        if (BaseOpcode != PPC::ADDItocL)
           continue;
 
-        if (!HBase.isMachineOpcode() ||
-            HBase.getMachineOpcode() != PPC::ADDIStocHA8)
+        if (!RegOperand.isMachineOpcode() ||
+            RegOperand.getMachineOpcode() != PPC::ADDIStocHA8)
           continue;
 
-        if (!Base.hasOneUse() || !HBase.hasOneUse())
+        if (!Base.hasOneUse() || !RegOperand.hasOneUse())
           continue;
 
-        SDValue HImmOpnd = HBase.getOperand(1);
+        SDValue HImmOpnd = RegOperand.getOperand(1);
         if (HImmOpnd != ImmOpnd)
           continue;
 
+        HBase = RegOperand;
         UpdateHBase = true;
       }
     } else {
       // If we're directly folding the addend from an addi instruction, then:
       //  1. In general, the offset on the memory access must be zero.
       //  2. If the addend is a constant, then it can be combined with a
       //     non-zero offset, but only if the result meets the encoding
       //     requirements.
       if (auto *C = dyn_cast<ConstantSDNode>(ImmOpnd)) {
         Offset += C->getSExtValue();
 
         if (RequiresMod4Offset && (Offset % 4) != 0)
           continue;
 
         if (!isInt<16>(Offset))
           continue;
 
         ImmOpnd = CurDAG->getTargetConstant(Offset, SDLoc(ImmOpnd),
                                             ImmOpnd.getValueType());
       } else if (Offset != 0) {
         continue;
       }
     }
 
-    // We found an opportunity.  Reverse the operands from the add
-    // immediate and substitute them into the load or store.  If
-    // needed, update the target flags for the immediate operand to
-    // reflect the necessary relocation information.
+    // We found an opportunity.  Forward the operands from the add
+    // immediate to the load or store.  If needed, update the target
+    // flags for the immediate operand to reflect the necessary
+    // relocation information.
     LLVM_DEBUG(dbgs() << "Folding add-immediate into mem-op:\nBase:    ");
     LLVM_DEBUG(Base->dump(CurDAG));
     LLVM_DEBUG(dbgs() << "\nN: ");
     LLVM_DEBUG(N->dump(CurDAG));
     LLVM_DEBUG(dbgs() << "\n");
 
     // If the relocation information isn't already present on the
     // immediate operand, add it now.
     if (ReplaceFlags) {
       if (GlobalAddressSDNode *GA = dyn_cast<GlobalAddressSDNode>(ImmOpnd)) {
         SDLoc dl(GA);
         const GlobalValue *GV = GA->getGlobal();
         Align Alignment = GV->getPointerAlignment(CurDAG->getDataLayout());
         // We can't perform this optimization for data whose alignment
         // is insufficient for the instruction encoding.
+        // TODO FIXME Verify and document why the offset must be a multiple of
+        // 4 when the aligment is less than 4. It is not about the encoding of
+        // the instruction: the value of Offset comes directly from the original
+        // load/store instruction on the path that reaches this check.
         if (Alignment < 4 && (RequiresMod4Offset || (Offset % 4) != 0)) {
           LLVM_DEBUG(dbgs() << "Rejected this candidate for alignment.\n\n");
           continue;
         }
         ImmOpnd = CurDAG->getTargetGlobalAddress(GV, dl, MVT::i64, Offset, Flags);
       } else if (ConstantPoolSDNode *CP =
                  dyn_cast<ConstantPoolSDNode>(ImmOpnd)) {
         const Constant *C = CP->getConstVal();
         ImmOpnd = CurDAG->getTargetConstantPool(C, MVT::i64, CP->getAlign(),
                                                 Offset, Flags);
       }
     }
 
-    const unsigned BaseOpcode = Base.getMachineOpcode();
-    // ADDItoc and ADDItoc8 are pseudos used exclusively by AIX small code
-    // model when a global is defined in the TOC.
-    const bool OpcodeIsAIXTocData =
-        BaseOpcode == PPC::ADDItoc || BaseOpcode == PPC::ADDItoc8;
-
     if (FirstOp == 1) // Store
-      if (OpcodeIsAIXTocData)
-        (void)CurDAG->UpdateNodeOperands(N, N->getOperand(0),
-                                         Base.getOperand(0), Base.getOperand(1),
-                                         N->getOperand(3));
-      else
-        (void)CurDAG->UpdateNodeOperands(N, N->getOperand(0), ImmOpnd,
-                                         Base.getOperand(0), N->getOperand(3));
+      (void)CurDAG->UpdateNodeOperands(N, N->getOperand(0), ImmOpnd, RegOperand,
     else // Load
-      if (OpcodeIsAIXTocData)
-        (void)CurDAG->UpdateNodeOperands(N, Base.getOperand(0),
-                                         Base.getOperand(1), N->getOperand(2));
-      else
-        (void)CurDAG->UpdateNodeOperands(N, ImmOpnd, Base.getOperand(0),
-                                         N->getOperand(2));
+      (void)CurDAG->UpdateNodeOperands(N, ImmOpnd, RegOperand,
 
     if (UpdateHBase)
       (void)CurDAG->UpdateNodeOperands(HBase.getNode(), HBase.getOperand(0),
                                        ImmOpnd);
 
     // The add-immediate may now be dead, in which case remove it.
     if (Base.getNode()->use_empty())
       CurDAG->RemoveDeadNode(Base.getNode());

llvm/test/CodeGen/PowerPC/aix-toc-data-offset.ll

+; RUN: llc -mtriple powerpc-ibm-aix-xcoff < %s | \
+; RUN: FileCheck %s
+
+; RUN: llc -mtriple powerpc64-ibm-aix-xcoff < %s | \
+; RUN: FileCheck %s
+
+@x = local_unnamed_addr global i32 218114560, align 4 #0
+
+define i32 @main() local_unnamed_addr {
+entry:
+  %0 = load i32, ptr @x, align 4
+  %shr = lshr i32 %0, 8
+  %and = and i32 %shr, 255
+  ret i32 %and
+}
+
+attributes #0 = { "toc-data" }
+
+; CHECK: la [[ADDR:[0-9]+]], x[TD](2)
+; CHECK: lbz {{.*}}, 2([[ADDR]])