[SystemZ] Bugfix in storeLoadCanUseBlockBinary()

[jonpa]

Summary

Check that the MemoryVT of LoadA matches that of LoadB.

This fixes https://bugs.llvm.org/show_bug.cgi?id=46239.

Does not affect benchmarks.

[uweigand]

Shouldn't the check be done inside canUseBlockOperation? I believe the other user (storeLoadCanUseMVC) really needs the same check.

[jonpa]

In D81671#2092717, @uweigand wrote:

Shouldn't the check be done inside canUseBlockOperation? I believe the other user (storeLoadCanUseMVC) really needs the same check.

My understanding is that storeLoadCanUseBlockBinary() is used as a pattern matching predicate for block memory operations of AND/OR/XOR. These involve two loads and one store. While canUseBlockOperation() has checks for a pair of a store and the load (of the other address), the second load (to the same address as the store) also needs to be checked, which is what this patch does.

The other user of canUseBlockOperation() is storeLoadCanUseMVC() which however only involves one load and one store, and this is why there is no need for anything else in this case.

[uweigand]

In D81671#2094870, @jonpa wrote:

In D81671#2092717, @uweigand wrote:

Shouldn't the check be done inside canUseBlockOperation? I believe the other user (storeLoadCanUseMVC) really needs the same check.

My understanding is that storeLoadCanUseBlockBinary() is used as a pattern matching predicate for block memory operations of AND/OR/XOR. These involve two loads and one store. While canUseBlockOperation() has checks for a pair of a store and the load (of the other address), the second load (to the same address as the store) also needs to be checked, which is what this patch does.

The other user of canUseBlockOperation() is storeLoadCanUseMVC() which however only involves one load and one store, and this is why there is no need for anything else in this case.

Ah, you're right, I missed the existing check in canUseBlockOperation. This patch is OK then.

llvm/lib/Target/SystemZ/SystemZISelDAGToDAG.cpp

bool SystemZDAGToDAGISel::storeLoadCanUseMVC(SDNode *N) const {
   return canUseBlockOperation(Store, Load);
 }
 
 bool SystemZDAGToDAGISel::storeLoadCanUseBlockBinary(SDNode *N,
                                                      unsigned I) const {
   auto *StoreA = cast<StoreSDNode>(N);
   auto *LoadA = cast<LoadSDNode>(StoreA->getValue().getOperand(1 - I));
   auto *LoadB = cast<LoadSDNode>(StoreA->getValue().getOperand(I));
-  return !LoadA->isVolatile() && canUseBlockOperation(StoreA, LoadB);
+  return !LoadA->isVolatile() && LoadA->getMemoryVT() == LoadB->getMemoryVT() &&
+         canUseBlockOperation(StoreA, LoadB);
 }
 
 void SystemZDAGToDAGISel::Select(SDNode *Node) {
   // If we have a custom node, we already have selected!
   if (Node->isMachineOpcode()) {
     LLVM_DEBUG(errs() << "== "; Node->dump(CurDAG); errs() << "\n");
     Node->setNodeId(-1);
     return;

llvm/test/CodeGen/SystemZ/and-08.ll

 ; CHECK: br %r14
   %val = load i64, i64 *%ptr1, align 2, !tbaa !3
   %old = load i64, i64 *%ptr2, align 2, !tbaa !3
   %and = and i64 %old, %val
   store i64 %and, i64 *%ptr2, align 2, !tbaa !3
   ret void
 }
 
+; Test a case where one of the loads are optimized by the DAGCombiner to a
+; zero-extending load of half the original size.
+define void @f27(i16* noalias %ptr1, i16* noalias %ptr2) {
+; CHECK-LABEL: f27:
+; CHECK-NOT: nc
+; CHECK: br %r14
+entry:
+  %0 = load i16, i16 *%ptr1, align 2
+  %1 = lshr i16 %0, 8
+  %2 = load i16, i16 *%ptr2, align 2
+  %and7 = and i16 %1, %2
+  store i16 %and7, i16 *%ptr1, align 2
+  ret void
+}
+
 !0 = !{ !"root" }
 !1 = !{ !"set1", !0 }
 !2 = !{ !"set2", !0 }
 !3 = !{ !1, !1, i64 0}
 !4 = !{ !2, !2, i64 0}