[SystemZ] Prevent Merging Bitcast with non-normal loads

[niravd]

Summary

Fixes PR32505.

[uweigand]

I guess this is OK with me, but I admit I don't quite understand why this would make any difference ... As far as I can see, the flavor of DAG.getLoad currently used would use the existing MemOperand, including its existing alignment, while the flavor in your patch creates a new MemOperand using the passed alignment value which the patch also gets from the current MemOperand. Either way we ought to end up with the same alignment ...

Would you mind explaining what I'm missing here?

[niravd]

I was slightly incorrectl it's not the alignment. I mixed up the various getLoads in my tracing. It's an issue with offset. I just pulled the corresponding call from the visitBITCAST since it's doing the same operation.

The original was indirectly calling:

SDValue getLoad(ISD::MemIndexedMode AM, ISD::LoadExtType ExtType, EVT VT,

const SDLoc &dl, SDValue Chain, SDValue Ptr, SDValue Offset,
EVT MemVT, MachineMemOperand *MMO);

with Offset set to undef which seems wrong. I suspect it should be zero.

The new version calls:

SDValue getLoad(EVT VT, const SDLoc &dl, SDValue Chain, SDValue Ptr,

MachinePointerInfo PtrInfo, unsigned Alignment = 0,
MachineMemOperand::Flags MMOFlags = MachineMemOperand::MONone,
const AAMDNodes &AAInfo = AAMDNodes(),
const MDNode *Ranges = nullptr);

[uweigand]

In D31609#718149, @niravd wrote:

I was slightly incorrectl it's not the alignment. I mixed up the various getLoads in my tracing. It's an issue with offset. I just pulled the corresponding call from the visitBITCAST since it's doing the same operation.

The original was indirectly calling:

SDValue getLoad(ISD::MemIndexedMode AM, ISD::LoadExtType ExtType, EVT VT,
                  const SDLoc &dl, SDValue Chain, SDValue Ptr, SDValue Offset,
                  EVT MemVT, MachineMemOperand *MMO);

with Offset set to undef which seems wrong. I suspect it should be zero.

According to the comments and assertions in ::getLoad, Offset must be Undef for non-indexed loads.

The new version calls:

SDValue getLoad(EVT VT, const SDLoc &dl, SDValue Chain, SDValue Ptr,
                 MachinePointerInfo PtrInfo, unsigned Alignment = 0,
                 MachineMemOperand::Flags MMOFlags = MachineMemOperand::MONone,
                 const AAMDNodes &AAInfo = AAMDNodes(),
                 const MDNode *Ranges = nullptr);

This likewise forwards to another version of getLoad specifying an unindexed load and an Offset of Undef, so I'm afraid I still don't see any difference:

SDValue SelectionDAG::getLoad(EVT VT, const SDLoc &dl, SDValue Chain,
                              SDValue Ptr, MachinePointerInfo PtrInfo,
                              unsigned Alignment,
                              MachineMemOperand::Flags MMOFlags,
                              const AAMDNodes &AAInfo, const MDNode *Ranges) {
  SDValue Undef = getUNDEF(Ptr.getValueType());
  return getLoad(ISD::UNINDEXED, ISD::NON_EXTLOAD, VT, dl, Chain, Ptr, Undef,
                 PtrInfo, VT, Alignment, MMOFlags, AAInfo, Ranges);
}

I'm not trying to be difficult here :-) I'd just like to understand what's going on to make sure there is no problem lurking with any of the other getLoad calls in the SystemZ back-end ...

[jonpa]

I don't follow how this relates to Offset (which seems to be 1 w or w/out patch)..?

The assert that triggers says 'memvt.getStoreSize() <= MMO->getSize() && "Size mismatch!"', which to me means that when calling getLoad(), the memory operand must have a size that is at least as big as the the size in memory of the load VT.

At trunk, the MMO Size is 1, as indicated on the DAG dump:

t0: ch = EntryToken
  t27: i32,ch = load<LD1[undef+1], sext from i8> t0, undef:i64, undef:i64
t22: f32 = bitcast t27

The load is going to load 1 byte (LD1), and then sext to i32.

With the trunk call to getLoad, we try to make a new Load of type f32 with a memory operand of size 1, which triggers the assert.

The patch instead calls getLoad (line 5621) with VT==f32, which calls getLoad (line 5551) with MemVT==f32. This method then creates a new MachineMemOperand of size 'MemVT.getStoreSize()', which is 4. Therefore this works.

This however means we are now loading 4 bytes as a float, instead of 1 byte which is sign-extended and then bitcast. I am guessing this must be wrong, or?

[jonpa]

On the other hand, it's a load from an undef address, so maybe that isn't illegal in this case...?

[uweigand]

Ah, I think the problem is simply that this piece of code in lowerBITCAST does not expect to be handling extending loads at all, and does the wrong thing for them. It probably just should verify this, and fall through to the rest of the code if the load is actually an extending one ...

[niravd]

In D31609#718978, @uweigand wrote:

Ah, I think the problem is simply that this piece of code in lowerBITCAST does not expect to be handling extending loads at all, and does the wrong thing for them. It probably just should verify this, and fall through to the rest of the code if the load is actually an extending one ...

This seems reasonable. We can change line 2794 to check isNormalLoad(N) and from my quick look at SystemZISelLowering it looks like Extended Loads have no direct realization.

[jonpa]

In D31609#719019, @niravd wrote:

In D31609#718978, @uweigand wrote:

Ah, I think the problem is simply that this piece of code in lowerBITCAST does not expect to be handling extending loads at all, and does the wrong thing for them. It probably just should verify this, and fall through to the rest of the code if the load is actually an extending one ...

This seems reasonable. We can change line 2794 to check isNormalLoad(N) and from my quick look at SystemZISelLowering it looks like Extended Loads have no direct realization.

Yes - I just tried

• return DAG.getLoad(ResVT, DL, LoadN->getChain(), LoadN->getBasePtr(),
• LoadN->getMemOperand());

+ if (LoadN->getExtensionType() == ISD::NON_EXTLOAD)
+ return DAG.getLoad(ResVT, DL, LoadN->getChain(), LoadN->getBasePtr(),
+ LoadN->getMemOperand());

And also had to add the -mcpu=zEC12 to the test (at least on my x86 laptop):
+; RUN: llc -mtriple=s390x-linux-gnu -mcpu=zEC12 -o - %s | FileCheck %s

We should basically not get a new Load SDNode if the load is extending.

/Jonas

[jonpa]

-    return DAG.getLoad(ResVT, DL, LoadN->getChain(), LoadN->getBasePtr(),
-                       LoadN->getMemOperand());
+    if (LoadN->getExtensionType() == ISD::NON_EXTLOAD)
+      return DAG.getLoad(ResVT, DL, LoadN->getChain(), LoadN->getBasePtr(),
+                         LoadN->getMemOperand());

sorry - this is with formatting

[niravd]

update to prevent extended / indexed loads from being merged with bitcast.

[niravd]

Patch also updated test to remove undef input address. Looks right to me.

[uweigand]

LGTM, thanks!

llvm/trunk/lib/Target/SystemZ/SystemZISelLowering.cpp

SDValue SystemZTargetLowering::lowerBITCAST(SDValue Op,
   SDValue In = Op.getOperand(0);
   EVT InVT = In.getValueType();
   EVT ResVT = Op.getValueType();
 
   // Convert loads directly.  This is normally done by DAGCombiner,
   // but we need this case for bitcasts that are created during lowering
   // and which are then lowered themselves.
   if (auto *LoadN = dyn_cast<LoadSDNode>(In))
+    if (ISD::isNormalLoad(LoadN))
       return DAG.getLoad(ResVT, DL, LoadN->getChain(), LoadN->getBasePtr(),
                          LoadN->getMemOperand());
 
   if (InVT == MVT::i32 && ResVT == MVT::f32) {
     SDValue In64;
     if (Subtarget.hasHighWord()) {
       SDNode *U64 = DAG.getMachineNode(TargetOpcode::IMPLICIT_DEF, DL,
                                        MVT::i64);
       In64 = DAG.getTargetInsertSubreg(SystemZ::subreg_h32, DL,
                                        MVT::i64, SDValue(U64, 0), In);

llvm/trunk/test/CodeGen/SystemZ/pr32505.ll

+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc -mcpu=zEC12 -o - %s | FileCheck %s
+
+target triple = "s390x-ibm-linux"
+
+define <2 x float> @pr32505(<2 x i8> * %a) {
+; CHECK-LABEL: pr32505:
+; CHECK:       # BB#0:
+; CHECK-NEXT:    lbh %r0, 0(%r2)
+; CHECK-NEXT:    ldgr %f0, %r0
+; CHECK-NEXT:    lbh %r0, 1(%r2)
+; CHECK-NEXT:    ldgr %f2, %r0
+; CHECK-NEXT:    # kill: %F0S<def> %F0S<kill> %F0D<kill>
+; CHECK-NEXT:    # kill: %F2S<def> %F2S<kill> %F2D<kill>
+; CHECK-NEXT:    br %r14
+  %L17 = load <2 x i8>, <2 x i8>* %a
+  %Se21 = sext <2 x i8> %L17 to <2 x i32>
+  %BC = bitcast <2 x i32> %Se21 to <2 x float>
+  ret <2 x float> %BC
+}