[ARM] Improve codegen of volatile load/store of i64

[vhscampos]

Summary

Instead of generating two i32 instructions for each load or store of a volatile
i64 value (two LDRs or STRs), now emit LDRD/STRD.

These improvements cover architectures implementing ARMv5TE or Thumb-2.

The code generation explicitly deviates from using the register-offset
variant of LDRD/STRD. In this variant, the register allocated to the
register-offset cannot be reused in any of the remaining operands. Such
restriction seems to be non-trivial to implement in LLVM, thus it is
left as a to-do.

[efriedma]

The architecture specification provides limited guarantees here, but I guess this doesn't do any harm.

llvm/lib/Target/ARM/ARMISelLowering.cpp

8969

I'd prefer not to exclude extending loads here. Could lead to weird cases where we miss the transform.

8983

Please don't make MachineSDNodes this early; it might appear to mostly work, but other code is not expecting MachineSDNodes at this point. See ARMISelLowering.h for how to introduce a target-specific SDNode.

[vhscampos]

1. Not exclude extloads anymore.
2. Create new ARMISD nodes specific to load/store of dual registers.
3. Custom lower i64 volatile loads/stores to these new ARMISD nodes.

[efriedma]

llvm/lib/Target/ARM/ARMISelLowering.cpp

9039

Loads and stores should not have an "Offset" at this point; we don't form pre/post-indexed operations until after legalization. Maybe worth asserting "isUnindexed()". (Same for loads.)

[efriedma]

llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp

3487 ↗

(On Diff #229043)

You should be able to use TableGen patterns for these, I think. AArch64Prefetch is an example of something similar to what you want.

llvm/lib/Target/ARM/ARMISelLowering.cpp

8969

You still need to *handle* extending loads somehow...

Actually, maybe we don't mess with volatile loads in DAGCombine, and you don't need to implement it. In that case, it would still be nice to have an assertion, in case someone changes it at some point.

9027

We also don't want to restrict truncating stores.

[dmgreen]

llvm/lib/Target/ARM/ARMISelLowering.cpp

9041

Likewise, this can just say if (Subtarget->hasMVEIntegerOps() && (VT == MVT::v4i1 || VT == MVT::v8i1 || VT == MVT::v16i1))

The other cases (isTruncatingStore and isUnindexed) should not come up. If they do we should noisily fail with an assert.

9245

How come this is altered, but not the LowerPredicateLoad?

[vhscampos]

llvm/lib/Target/ARM/ARMISelLowering.cpp

9245

The custom lowering of loads and stores here is triggered by the DAG Type Legalizer, since i64 is not supported.

In DAGTypeLegalizer::CustomLowerNode(), custom lowering of loads is directed to ARMTargetLowering::ReplaceNodeResults(), which then calls LowerLOAD(), created in the present patch. The lowering of stores is the one that is directed to ARMTargetLowering::LowerOperation().

In summary, the custom lowering of loads because of illegal result types does not go through here, so I believe there's no need to have it changed in this point.

[vhscampos]

1. Move the custom SD nodes to TableGen.
2. Truncating stores not restricted anymore.
3. Remove isUnindexed() calls since they should return true always at this point.
4. Extend test to also check loads/stores that have an offset.

[vhscampos]

llvm/lib/Target/ARM/ARMISelLowering.cpp

8969

Your last comment on this wasn't clear to me. If after the latest change you still want me to add anything, please let me know.

[efriedma]

llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp

1279 ↗

(On Diff #230276)

Are you sure this range computation is right? The range should be multiples of 4 from -1020 to 1020.

[vhscampos]

Fixed the immediate range check to cover -1020 to 1020.

[dmgreen]

llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp

1279 ↗

(On Diff #230276)

It may be worth adding tests for cases that are close to or just over the boundary.

[vhscampos]

1. Update summary to have a better explanation of this patch.
2. Add a post-ISel hook to add register allocation hints to LDRD/STRD operands.
3. In the AArch32 case, move ISel from TableGen back to the C++ side. This is needed because we must have a custom lowering whenever LDRD/STRD selection would normally yield a register offset. The ARM Load/Store Optimizer is not able to handle LDRD/STRD's register offsets in the cases where LDRD/STRD must be reverted to LDM/STM. As such, the C++ instruction selection opts for not generating instructions with a register offset.
4. Improve test by testing several immediate boundary cases.

[efriedma]

llvm/lib/Target/ARM/ARMISelLowering.cpp

10749

Oh, I didn't notice this before.

There's a problem here: if we're trying to generate LDRD for the sake of the extra guarantees provided by the some targets, transforming that to LDM is wrong; it doesn't have the same guarantee, and therefore could cause unpredictable, subtle problems.

Probably we want to allocate a GPRPair, instead of allocating two separate registers and trying to tie them together with a hint. Maybe requires defining a new pseudo-instruction that takes a GPRPair instead of two GPRs.

Or I'd be okay with just restricting the optimization to Thumb2 for now, if you don't want to do the extra work right now.

[vhscampos]

Create ARM PseudoInsts that take a register pair operand. This way we can enforce the allocation requirement.

[efriedma]

llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp

3549 ↗

(On Diff #234045)

I think you could implement this pattern in TableGen (grep for REG_SEQUENCE in the ARM .td files). But probably not the corresponding load pattern, since the inverse opcode doesn't exist, so not a big deal either way.

llvm/lib/Target/ARM/ARMInstrInfo.td

2704 ↗

(On Diff #234045)

hasExtraDefRegAllocReq shouldn't be necessary; the regular ldrd/strd are weird because the register allocation constraint isn't expressed correctly by the operand types, but you don't have that problem here.

[vhscampos]

Changed STOREDUAL instruction selection to use TableGen patterns.
Removed one constraint from LOADDUAL and STOREDUAL.

[efriedma]

LGTM

[vvereschaka]

Hi Victor,

this commit breaks "LLVM::2010-05-03-OriginDIE.ll" test on "llvm-clang-win-x-armv7l" builder.
Here is the failed build http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l/builds/1851

Would you take a look?

Thanks,
Vlad.

[vhscampos]

I created a new patch fixing the issue: https://reviews.llvm.org/D71749

[nickdesaulniers]

Hi Victor, we're observing a crash in Clang when compiling the Linux kernel bisected to this commit. Can you please revert?
See the trace in https://ci.linaro.org/job/tcwg_kernel-bisect-llvm-master-arm-next-allmodconfig/62/artifact/artifacts/build-bbcf1c3496ce2bd1ed87e8fb15ad896e279633ce/console.log grep for stack dump.

[vhscampos]

I have reverted this commit.

Also, this revision was reopened so that I can append its fix here for reviewing.

[vhscampos]

This is the same patch as before but with fixes for the tests that regressed (as reported in the comments here).

The fix is to specify the address mode of the two pseudo instructions introduced.

[nickdesaulniers]

Thanks @vhscampos , I tested the new patch and could no longer reproduce the observed crashes. I'm not sure if I'm looking at the interdiff correctly, but consider adding additional test cases that properly describe and cover the breakage we observed.

[vhscampos]

Added one test to cover the issue related to loads/stores to a stack frame.

[nickdesaulniers]

@vhscampos sorry, we're getting new/different warnings now seemingly with this patch: https://github.com/ClangBuiltLinux/linux/issues/838

Warning: index register overlaps transfer register

[efriedma]

Apparently the ARM-mode LDRD is a bit more strange than I realized. From the ARM manual: if t2 == 15 || m == 15 || m == t || m == t2 then UNPREDICTABLE;. I guess we're managed to avoid running into this in the past by never generating the register form of ldrd.

It should be possible to express this constraint to the register allocator using @earlyclobber. (@earlyclobber is actually a little more conservative than we need, strictly speaking, but the difference probably doesn't matter too much.)

[vhscampos]

@nickdesaulniers @efriedma @nathanchance Apologies for missing the latest comments! Since this seems to be a blocker, I'd suggest that this change gets reverted until I am able to have a closer look at the issue of register overlapping. What do you think?

[nathanchance]

Please revert; I am happy to test a new revision to make sure there are no warnings but I don’t want this shipped in clang-10 and a revert is something that we can easily backport unless you can come up with a fix rather quickly.

[vhscampos]

Reverted in the master branch.

[hans]

In D70072#1865561, @vhscampos wrote:

Reverted in the master branch.

I see it was also reverted on the release/10.x branch 7996b49053f0508717f4a081d197ddc3073f4b5f. Thanks for keeping the branch in mind, but as mentioned in http://lists.llvm.org/pipermail/llvm-dev/2020-January/138295.html please check with me before pushing directly to it.

[vhscampos]

Changes:

1. Explictly avoid using the register-offset variant of LDRD/STRD. This variant has a limitation on register allocation: the register allocated to the register-offset cannot be reused in any of the remaining operands. I could not find an easy way to implement this in LLVM, so I left it as a to-do in the future.
2. Instruction selection of STRD was moved from TableGen to C++ because of point (1).
3. Updated tests to reflect these changes.

[vhscampos]

Can I please have this reviewed again? I have addressed the issues reported.

[nickdesaulniers]

Boot tested a clang built arm32 linux kernel in QEMU with this patch applied. Thanks for following up with fixes.

[efriedma]

LGTM with one minor comment.

llvm/lib/Target/ARM/ARMISelDAGToDAG.cpp

3622 ↗

(On Diff #249434)

The RegOffset check could use a comment explaining what it's doing, here and for STRD.

[vhscampos]

Added comment to explain the fallback to non-register-offset variants.

[vhscampos]

Improve the testcase which exercises loads and stores from stack. Now, wrong frame index replacements will be caught here.

[efriedma]

LGTM

llvm/lib/Target/ARM/ARMISelLowering.h

private:
     SDValue LowerREM(SDNode *N, SelectionDAG &DAG) const;
     SDValue LowerDYNAMIC_STACKALLOC(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerFP_ROUND(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerFP_EXTEND(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerFP_TO_INT(SDValue Op, SelectionDAG &DAG) const;
     SDValue LowerINT_TO_FP(SDValue Op, SelectionDAG &DAG) const;
     void lowerABS(SDNode *N, SmallVectorImpl<SDValue> &Results,
                   SelectionDAG &DAG) const;
+    void LowerLOAD(SDNode *N, SmallVectorImpl<SDValue> &Results,
+                   SelectionDAG &DAG) const;
 
     Register getRegisterByName(const char* RegName, EVT VT,
                                const MachineFunction &MF) const override;
 
     SDValue BuildSDIVPow2(SDNode *N, const APInt &Divisor, SelectionDAG &DAG,
                           SmallVectorImpl<SDNode *> &Created) const override;
 
     bool isFMAFasterThanFMulAndFAdd(EVT VT) const override;

llvm/lib/Target/ARM/ARMISelLowering.cpp

ARMTargetLowering::ARMTargetLowering(const TargetMachine &TM,
 
   setOperationAction(ISD::SHL_PARTS, MVT::i32, Custom);
   setOperationAction(ISD::SRA_PARTS, MVT::i32, Custom);
   setOperationAction(ISD::SRL_PARTS, MVT::i32, Custom);
   setOperationAction(ISD::SRL,       MVT::i64, Custom);
   setOperationAction(ISD::SRA,       MVT::i64, Custom);
   setOperationAction(ISD::INTRINSIC_VOID, MVT::Other, Custom);
   setOperationAction(ISD::INTRINSIC_WO_CHAIN, MVT::i64, Custom);
+  setOperationAction(ISD::LOAD, MVT::i64, Custom);
+  setOperationAction(ISD::STORE, MVT::i64, Custom);
 
   // MVE lowers 64 bit shifts to lsll and lsrl
   // assuming that ISD::SRL and SRA of i64 are already marked custom
   if (Subtarget->hasMVEIntegerOps())
     setOperationAction(ISD::SHL, MVT::i64, Custom);
 
   // Expand to __aeabi_l{lsl,lsr,asr} calls for Thumb1.
   if (Subtarget->isThumb1Only()) {
SDValue Load = DAG.getExtLoad(
       LD->getMemOperand());
   SDValue Pred = DAG.getNode(ARMISD::PREDICATE_CAST, dl, MVT::v16i1, Load);
   if (MemVT != MVT::v16i1)
     Pred = DAG.getNode(ISD::EXTRACT_SUBVECTOR, dl, MemVT, Pred,
                        DAG.getConstant(0, dl, MVT::i32));
   return DAG.getMergeValues({Pred, Load.getValue(1)}, dl);
 }
 
+void ARMTargetLowering::LowerLOAD(SDNode *N, SmallVectorImpl<SDValue> &Results,
+                                  SelectionDAG &DAG) const {
+  LoadSDNode *LD = cast<LoadSDNode>(N);
+  EVT MemVT = LD->getMemoryVT();
+
+  if (MemVT == MVT::i64 && Subtarget->hasV5TEOps() &&
+      !Subtarget->isThumb1Only() &&
+      LD->getExtensionType() == ISD::NON_EXTLOAD && LD->isVolatile()) {
+    SDLoc dl(N);
+    const SDValue &Offset = LD->isIndexed()
+                                ? LD->getOffset()
+                                : DAG.getTargetConstant(0, dl, MVT::i32);
+    unsigned OpCode = Subtarget->isThumb2() ? ARM::t2LDRDi8 : ARM::LDRD;
+    SmallVector<SDValue, 6> Ops = {LD->getBasePtr()};
+    if (!Subtarget->isThumb2()) {
+      Ops.push_back(DAG.getRegister(0, MVT::i32));
+    }
+    Ops.append({Offset,
+                DAG.getTargetConstant((uint64_t)ARMCC::AL, dl, MVT::i32),
+                DAG.getRegister(0, MVT::i32), LD->getChain()});
+    MachineSDNode *Result =
+        DAG.getMachineNode(OpCode, dl, {MVT::i32, MVT::i32, MVT::Other}, Ops);
+    MachineMemOperand *MemOp = cast<MemSDNode>(N)->getMemOperand();
+    DAG.setNodeMemRefs(cast<MachineSDNode>(Result), {MemOp});
+    SDValue Pair = DAG.getNode(ISD::BUILD_PAIR, dl, MVT::i64,
+                               SDValue(Result, 0), SDValue(Result, 1));
+    Results.append({Pair, SDValue(Result, 2) /* Chain */});
+  }
+}
+
 static SDValue LowerPredicateStore(SDValue Op, SelectionDAG &DAG) {
   StoreSDNode *ST = cast<StoreSDNode>(Op.getNode());
   EVT MemVT = ST->getMemoryVT();
   assert((MemVT == MVT::v4i1 || MemVT == MVT::v8i1 || MemVT == MVT::v16i1) &&
          "Expected a predicate type!");
   assert(MemVT == ST->getValue().getValueType());
   assert(!ST->isTruncatingStore() && "Expected a non-extending store");
   assert(ST->isUnindexed() && "Expected a unindexed store");
static SDValue LowerPredicateStore(SDValue Op, SelectionDAG &DAG) {
   }
   SDValue GRP = DAG.getNode(ARMISD::PREDICATE_CAST, dl, MVT::i32, Build);
   return DAG.getTruncStore(
       ST->getChain(), dl, GRP, ST->getBasePtr(),
       EVT::getIntegerVT(*DAG.getContext(), MemVT.getSizeInBits()),
       ST->getMemOperand());
 }
 
+static SDValue LowerSTORE(SDValue Op, SelectionDAG &DAG,
+                          const ARMSubtarget *Subtarget) {
+  StoreSDNode *ST = cast<StoreSDNode>(Op.getNode());
+  EVT MemVT = ST->getMemoryVT();
+
+  if (MemVT == MVT::i64 && Subtarget->hasV5TEOps() &&
+      !Subtarget->isThumb1Only() && !ST->isTruncatingStore() &&
+      ST->isVolatile()) {
+    SDNode *N = Op.getNode();
+    SDLoc dl(N);
+
+    SDValue Lo = DAG.getNode(ISD::EXTRACT_ELEMENT, dl, MVT::i32, ST->getValue(),
+                             DAG.getTargetConstant(0, dl, MVT::i32));
+    SDValue Hi = DAG.getNode(ISD::EXTRACT_ELEMENT, dl, MVT::i32, ST->getValue(),
+                             DAG.getTargetConstant(1, dl, MVT::i32));
+    const SDValue &Offset = ST->isIndexed()
+                                ? ST->getOffset()
+                                : DAG.getTargetConstant(0, dl, MVT::i32);
+    unsigned OpCode = Subtarget->isThumb2() ? ARM::t2STRDi8 : ARM::STRD;
+    SmallVector<SDValue, 8> Ops = {Lo, Hi, ST->getBasePtr()};
+    if (!Subtarget->isThumb2()) {
+      Ops.push_back(DAG.getRegister(0, MVT::i32));
+    }
+    Ops.append({Offset,
+                DAG.getTargetConstant((uint64_t)ARMCC::AL, dl, MVT::i32),
+                DAG.getRegister(0, MVT::i32), ST->getChain()});
+    MachineSDNode *Result = DAG.getMachineNode(OpCode, dl, MVT::Other, Ops);
+    MachineMemOperand *MemOp = cast<MemSDNode>(N)->getMemOperand();
+    DAG.setNodeMemRefs(cast<MachineSDNode>(Result), {MemOp});
+
+    return SDValue(Result, 0);
+  } else if ((MemVT == MVT::v4i1 || MemVT == MVT::v8i1 ||
+              MemVT == MVT::v16i1) &&
+             !ST->isTruncatingStore() && ST->isUnindexed()) {
+    return LowerPredicateStore(Op, DAG);
+  }
+
+  return SDValue();
+}
+
 static SDValue LowerMLOAD(SDValue Op, SelectionDAG &DAG) {
   MaskedLoadSDNode *N = cast<MaskedLoadSDNode>(Op.getNode());
   MVT VT = Op.getSimpleValueType();
   SDValue Mask = N->getMask();
   SDValue PassThru = N->getPassThru();
   SDLoc dl(Op);
 
   auto IsZero = [](SDValue PassThru) {
SDValue ARMTargetLowering::LowerOperation(SDValue Op, SelectionDAG &DAG) const {
   case ISD::USUBO:
     return LowerUnsignedALUO(Op, DAG);
   case ISD::SADDSAT:
   case ISD::SSUBSAT:
     return LowerSADDSUBSAT(Op, DAG, Subtarget);
   case ISD::LOAD:
     return LowerPredicateLoad(Op, DAG);
   case ISD::STORE:
-    return LowerPredicateStore(Op, DAG);
+    return LowerSTORE(Op, DAG, Subtarget);
   case ISD::MLOAD:
     return LowerMLOAD(Op, DAG);
   case ISD::ATOMIC_LOAD:
   case ISD::ATOMIC_STORE:  return LowerAtomicLoadStore(Op, DAG);
   case ISD::FSINCOS:       return LowerFSINCOS(Op, DAG);
   case ISD::SDIVREM:
   case ISD::UDIVREM:       return LowerDivRem(Op, DAG);
   case ISD::DYNAMIC_STACKALLOC:
void ARMTargetLowering::ReplaceNodeResults(SDNode *N,
   case ISD::ATOMIC_CMP_SWAP:
     ReplaceCMP_SWAP_64Results(N, Results, DAG);
     return;
   case ISD::INTRINSIC_WO_CHAIN:
     return ReplaceLongIntrinsic(N, Results, DAG);
   case ISD::ABS:
      lowerABS(N, Results, DAG);
      return ;
-
+  case ISD::LOAD:
+    LowerLOAD(N, Results, DAG);
+    break;
   }
   if (Res.getNode())
     Results.push_back(Res);
 }
 
 //===----------------------------------------------------------------------===//
 //                           ARM Scheduler Hooks
 //===----------------------------------------------------------------------===//
if (MI.getOpcode() == ARM::MEMCPY) {
     attachMEMCPYScratchRegs(Subtarget, MI, Node);
     return;
   }
 
   const MCInstrDesc *MCID = &MI.getDesc();
   // Adjust potentially 's' setting instructions after isel, i.e. ADC, SBC, RSB,
   // RSC. Coming out of isel, they have an implicit CPSR def, but the optional
   // operand is still set to noreg. If needed, set the optional operand's
   // register to CPSR, and remove the redundant implicit def.
   //
   // e.g. ADCS (..., implicit-def CPSR) -> ADC (... opt:def CPSR).
 
   // Rename pseudo opcodes.
   unsigned NewOpc = convertAddSubFlagsOpcode(MI.getOpcode());
   unsigned ccOutIdx;
   if (NewOpc) {
     const ARMBaseInstrInfo *TII = Subtarget->getInstrInfo();

llvm/test/CodeGen/ARM/i64_volatile_load_store.ll

+; RUN: llc -mtriple=armv5e-arm-none-eabi %s -o - | FileCheck %s --check-prefix=CHECK-ARMV5TE
+; RUN: llc -mtriple=thumbv6t2-arm-none-eabi %s -o - | FileCheck %s --check-prefix=CHECK-T2
+; RUN: llc -mtriple=armv4t-arm-none-eabi %s -o - | FileCheck %s --check-prefix=CHECK-ARMV4T
+
+@x = common dso_local global i64 0, align 8
+@y = common dso_local global i64 0, align 8
+
+define void @test() {
+entry:
+; CHECK-ARMV5TE: ldr [[ADDR0:r[0-9]+]]
+; CHECK-ARMV5TE: ldr [[ADDR1:r[0-9]+]]
+; CHECK-ARMV5TE: ldrd [[R0:r[0-9]+]], [[R1:r[0-9]+]], {{\[}}[[ADDR0]]]
+; CHECK-ARMV5TE: strd [[R0]], [[R1]], {{\[}}[[ADDR1]]]
+; CHECK-T2:      movw [[ADDR0:r[0-9]+]], :lower16:x
+; CHECK-T2:      movw [[ADDR1:r[0-9]+]], :lower16:y
+; CHECK-T2:      movt [[ADDR0]], :upper16:x
+; CHECK-T2:      movt [[ADDR1]], :upper16:y
+; CHECK-T2:      ldrd [[R0:r[0-9]+]], [[R1:r[0-9]+]], {{\[}}[[ADDR0]]]
+; CHECK-T2:      strd [[R0]], [[R1]], {{\[}}[[ADDR1]]]
+; CHECK-ARMV4T:  ldr [[ADDR0:r[0-9]+]]
+; CHECK-ARMV4T:  ldr [[ADDR1:r[0-9]+]]
+; CHECK-ARMV4T:  ldr [[R1:r[0-9]+]], {{\[}}[[ADDR0]]]
+; CHECK-ARMV4T:  ldr [[R0:r[0-9]+]], {{\[}}[[ADDR0]], #4]
+; CHECK-ARMV4T:  str [[R0]], {{\[}}[[ADDR1]], #4]
+; CHECK-ARMV4T:  str [[R1]], {{\[}}[[ADDR1]]]
+  %0 = load volatile i64, i64* @x, align 8
+  store volatile i64 %0, i64* @y, align 8
+  ret void
+}
+