[PATCH] D30529: [RFC][GlobalISel] Enable legalizing non-power-of-2 sized types.

[Tim Northover via Phabricator via llvm-commits]

t.p.northover added a comment.

Hi Kristof,

Thanks for working on this and I'm really sorry it took so long to reply.

I like the basic structure and I think it should be able to represent everything we need. I originally thought that mixing the strategies with `setAction` calls was clunky, but I assume almost all of that is going to go away once sensible default strategies are in place for all the operations?



================
Comment at: lib/CodeGen/GlobalISel/LegalizerInfo.cpp:45-46
 
-  DefaultActions[TargetOpcode::G_ADD] = NarrowScalar;
-  DefaultActions[TargetOpcode::G_LOAD] = NarrowScalar;
-  DefaultActions[TargetOpcode::G_STORE] = NarrowScalar;
+  setLegalizeScalarToDifferentSizeStrategy(
+      TargetOpcode::G_ADD, 0, narrowToSmallerAndUnsupportedIfTooSmall);
+  setLegalizeScalarToDifferentSizeStrategy(
----------------
This should probably be widen-then-narrow, but I assume it's like this to minimize the functional diff.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerInfo.cpp:81-89
+        SizeAndActionsVec &v =
+            Type.isPointer()
+                ? AddressSpace2SpecifiedActions[Type.getAddressSpace()]
+                : Type.isVector()
+                      ? ElemSize2SpecifiedActions[Type.getElementType()
+                                                      .getSizeInBits()]
+
----------------
Since v is only used for the push_back maybe just do it directly with ifs:

    auto SizeAction = std::make_pair(Type.getSizeInBits(), Action);
    if (Type.isPointer())
      AddressSpace2SpecifiedActions[Type.getAddressSpace()].push_back(SizeAction);
    else if (Type.isVector())
      ElemSize2SpecifiedActions[Type.getElementType().getSizeInBits()].push_back(SizeAction);
    else
      ScalarSpecifiedActions.push_back(SizeAction);


================
Comment at: lib/CodeGen/GlobalISel/LegalizerInfo.cpp:253-260
+  int VecIdx = -1;
+  for (int i = Vec.size() - 1; i >= 0; --i) {
+    const uint32_t BitSize = Vec[i].first;
+    if (BitSize <= Size) {
+      VecIdx = i;
+      break;
+    }
----------------
I think this is approximately

    VecIdx = std::lower_bound(Vec.begin(), Vec.end()) - Vec.begin();

which has added binary-search goodness.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerInfo.cpp:275-277
+    // FIXME: do we really need a loop here? Or could we assert at construction
+    // time that there cannot be 2 consecutive actions in a SizeAndActionsVec
+    // for which NeedsLegalizingToDifferentSize() is true.
----------------
I think the assertion would be reasonable.


================
Comment at: lib/Target/AArch64/AArch64LegalizerInfo.cpp:107-108
+
+static LegalizerInfo::SizeAndActionsVec
+widen_1_8_16_narrowLarger(const LegalizerInfo::SizeAndActionsVec &v) {
+  assert(v.size() >= 1);
----------------
I think this is the same as `widen_1_8_16_narrowToLargest` isn't it?


https://reviews.llvm.org/D30529