GlobalISel: memSizeNotByteSizePow2 legality helper

[arsenm]

Summary

This is really a replacement for memSizeInBytesNotPow2 that actually
does what most every target wants. In particular, since s1 rounds to 1
byte, it wasn't lowered by this predicate. This results in targets
needing to think harder and add more matchers to catch all the
degenerate cases.

Also small bug fix that prevented the correct insertion of
G_ASSERT_ZEXT in the AArch64 use case.

[arsenm]

Missed test update

[paquette]

llvm/lib/CodeGen/GlobalISel/GISelKnownBits.cpp

569 ↗

(On Diff #421983)

wrong patch?

[arsenm]

Correct patch

[paquette]

Also small bug fix that prevented the correct insertion of G_ASSERT_ZEXT in the AArch64 use case.

This looks like it's 32 bit ARM not AArch64?

Anyway this looks reasonable to me.

[arsenm]

In D123518#3444061, @paquette wrote:

Also small bug fix that prevented the correct insertion of G_ASSERT_ZEXT in the AArch64 use case.

This looks like it's 32 bit ARM not AArch64?

No, the fix prevented a regression in the AArch64 case. The opposite improvement was made to the ARM one

[paquette]

Oh, I see

[arsenm]

d1f97a341958bb658e03bd8a01e9f19a9924d114

llvm/include/llvm/CodeGen/GlobalISel/LegalizerInfo.h

 /// True iff the first type index has a larger total bit size than second type
 /// index.
 LegalityPredicate largerThan(unsigned TypeIdx0, unsigned TypeIdx1);
 
 /// True iff the first type index has a smaller total bit size than second type
 /// index.
 LegalityPredicate smallerThan(unsigned TypeIdx0, unsigned TypeIdx1);
 
-/// True iff the specified MMO index has a size that is not a power of 2
+/// True iff the specified MMO index has a size (rounded to bytes) that is not a
+/// power of 2.
 LegalityPredicate memSizeInBytesNotPow2(unsigned MMOIdx);
+
+/// True iff the specified MMO index has a size that is not an even byte size,
+/// or that even byte size is not a power of 2.
+LegalityPredicate memSizeNotByteSizePow2(unsigned MMOIdx);
+
 /// True iff the specified type index is a vector whose element count is not a
 /// power of 2.
 LegalityPredicate numElementsNotPow2(unsigned TypeIdx);
 /// True iff the specified MMO index has at an atomic ordering of at Ordering or
 /// stronger.
 LegalityPredicate atomicOrderingAtLeastOrStrongerThan(unsigned MMOIdx,
                                                       AtomicOrdering Ordering);
 } // end namespace LegalityPredicates
#endif
   LegalizeRuleSet &unsupportedFor(std::initializer_list<LLT> Types) {
     return actionFor(LegalizeAction::Unsupported, Types);
   }
 
   LegalizeRuleSet &unsupportedIfMemSizeNotPow2() {
     return actionIf(LegalizeAction::Unsupported,
                     LegalityPredicates::memSizeInBytesNotPow2(0));
   }
+
+  /// Lower a memory operation if the memory size, rounded to bytes, is not a
+  /// power of 2. For example, this will not trigger for s1 or s7, but will for
+  /// s24.
   LegalizeRuleSet &lowerIfMemSizeNotPow2() {
     return actionIf(LegalizeAction::Lower,
                     LegalityPredicates::memSizeInBytesNotPow2(0));
   }
 
+  /// Lower a memory operation if the memory access size is not a round power of
+  /// 2 byte size. This is stricter than lowerIfMemSizeNotPow2, and more likely
+  /// what you want (e.g. this will lower s1, s7 and s24).
+  LegalizeRuleSet &lowerIfMemSizeNotByteSizePow2() {
+    return actionIf(LegalizeAction::Lower,
+                    LegalityPredicates::memSizeNotByteSizePow2(0));
+  }
+
   LegalizeRuleSet &customIf(LegalityPredicate Predicate) {
     // We have no choice but conservatively assume that a custom action with a
     // free-form user provided Predicate properly handles all type indices:
     markAllIdxsAsCovered();
     return actionIf(LegalizeAction::Custom, Predicate);
   }
   LegalizeRuleSet &customFor(std::initializer_list<LLT> Types) {
     return actionFor(LegalizeAction::Custom, Types);

llvm/lib/CodeGen/GlobalISel/LegalityPredicates.cpp

 }
 
 LegalityPredicate LegalityPredicates::memSizeInBytesNotPow2(unsigned MMOIdx) {
   return [=](const LegalityQuery &Query) {
     return !isPowerOf2_32(Query.MMODescrs[MMOIdx].MemoryTy.getSizeInBytes());
   };
 }
 
+LegalityPredicate LegalityPredicates::memSizeNotByteSizePow2(unsigned MMOIdx) {
+  return [=](const LegalityQuery &Query) {
+    const LLT MemTy = Query.MMODescrs[MMOIdx].MemoryTy;
+    return !MemTy.isByteSized() || !isPowerOf2_32(MemTy.getSizeInBytes());
+  };
+}
+
 LegalityPredicate LegalityPredicates::numElementsNotPow2(unsigned TypeIdx) {
   return [=](const LegalityQuery &Query) {
     const LLT QueryTy = Query.Types[TypeIdx];
     return QueryTy.isVector() && !isPowerOf2_32(QueryTy.getNumElements());
   };
 }
 
 LegalityPredicate LegalityPredicates::atomicOrderingAtLeastOrStrongerThan(
     unsigned MMOIdx, AtomicOrdering Ordering) {
   return [=](const LegalityQuery &Query) {
     return isAtLeastOrStrongerThan(Query.MMODescrs[MMOIdx].Ordering, Ordering);
   };
 }

llvm/lib/CodeGen/GlobalISel/LegalizerHelper.cpp

if (MemSizeInBits != MemStoreSizeInBits) {
     if (MemStoreSizeInBits > DstTy.getSizeInBits()) {
       LoadTy = WideMemTy;
       LoadReg = MRI.createGenericVirtualRegister(WideMemTy);
     }
 
     if (isa<GSExtLoad>(LoadMI)) {
       auto NewLoad = MIRBuilder.buildLoad(LoadTy, PtrReg, *NewMMO);
       MIRBuilder.buildSExtInReg(LoadReg, NewLoad, MemSizeInBits);
-    } else if (isa<GZExtLoad>(LoadMI) || WideMemTy == DstTy) {
+    } else if (isa<GZExtLoad>(LoadMI) || WideMemTy == LoadTy) {
       auto NewLoad = MIRBuilder.buildLoad(LoadTy, PtrReg, *NewMMO);
       // The extra bits are guaranteed to be zero, since we stored them that
       // way.  A zext load from Wide thus automatically gives zext from MemVT.
       MIRBuilder.buildAssertZExt(LoadReg, NewLoad, MemSizeInBits);
     } else {
       MIRBuilder.buildLoad(LoadReg, PtrReg, *NewMMO);
     }
 

llvm/lib/Target/AArch64/GISel/AArch64LegalizerInfo.cpp

getActionDefinitionsBuilder(G_LOAD)
                                  {v4s16, p0, s64, 8},
                                  {v8s16, p0, s128, 8},
                                  {v2s32, p0, s64, 8},
                                  {v4s32, p0, s128, 8},
                                  {v2s64, p0, s128, 8}})
       // These extends are also legal
       .legalForTypesWithMemDesc({{s32, p0, s8, 8}, {s32, p0, s16, 8}})
       .widenScalarToNextPow2(0, /* MinSize = */8)
-      .lowerIfMemSizeNotPow2()
+      .lowerIfMemSizeNotByteSizePow2()
       .clampScalar(0, s8, s64)
       .narrowScalarIf([=](const LegalityQuery &Query) {
         // Clamp extending load results to 32-bits.
         return Query.Types[0].isScalar() &&
           Query.Types[0] != Query.MMODescrs[0].MemoryTy &&
           Query.Types[0].getSizeInBits() > 32;
         },
         changeTo(0, s32))
-      // Lower any any-extending loads left into G_ANYEXT and G_LOAD
-      .lowerIf([=](const LegalityQuery &Query) {
-        return Query.Types[0] != Query.MMODescrs[0].MemoryTy;
-      })
       .clampMaxNumElements(0, s8, 16)
       .clampMaxNumElements(0, s16, 8)
       .clampMaxNumElements(0, s32, 4)
       .clampMaxNumElements(0, s64, 2)
       .clampMaxNumElements(0, p0, 2)
       .customIf(IsPtrVecPred)
       .scalarizeIf(typeIs(0, v2s16), 0);
 

llvm/test/CodeGen/ARM/GlobalISel/arm-legalize-load-store.mir

 regBankSelected: false
 selected:        false
 tracksRegLiveness: true
 body:             |
   bb.0:
     liveins: $r0
 
     ; CHECK: [[LD:%[0-9]+]]:_(s8) = G_LOAD %0(p0) :: (load (s8))
+    ; CHECK: [[ASSERTZ:%[0-9]+]]:_(s8) = G_ASSERT_ZEXT [[LD]], 1
     ; CHECK: [[ONE:%[0-9]+]]:_(s32) = G_CONSTANT i32 1
-    ; CHECK: [[EXT:%[0-9]+]]:_(s32) = G_ANYEXT [[LD]]
+    ; CHECK: [[EXT:%[0-9]+]]:_(s32) = G_ANYEXT [[ASSERTZ]]
     ; CHECK: [[AND:%[0-9]+]]:_(s32) = G_AND [[EXT]], [[ONE]]
     ; CHECK: [[TRUNC:%[0-9]+]]:_(s8) = G_TRUNC [[AND]]
     ; CHECK: G_STORE [[TRUNC]](s8), {{%[0-9]+}}(p0) :: (store (s8))
     %0:_(p0) = COPY $r0
     %5:_(s1) = G_LOAD %0(p0) :: (load (s1))
     G_STORE %5(s1), %0(p0) :: (store (s1))
     BX_RET 14, $noreg
 ...