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

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added a comment.

In https://reviews.llvm.org/D30529#820847, @t.p.northover wrote:

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


No problem, and thanks very much for the review!

> 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?

I'm assuming you're talking about how the target defines legality in the TargetLegalizerInfo, right? Yes, I expect most of the strategy specifications to go away there in follow-up patches that don't aim to be as NFC-ish as this one, introducing more default strategies.
I agree that the mixing of setAction and strategies and how they work together isn't fully trivial, but it seems to me that they probably would work well in practice, which is why I also liked this basic structure. So, probably, once this lands, a brief explanation might be needed somewhere on http://llvm.org/docs/GlobalISel.html - enough for target authors to understand how the setAction and SizeChangingStrategies work together.



================
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(
----------------
t.p.northover wrote:
> This should probably be widen-then-narrow, but I assume it's like this to minimize the functional diff.
Hmmm, I can't tell of the top of my head. I'll look into it.


================
Comment at: lib/CodeGen/GlobalISel/LegalizerInfo.cpp:81-89
+        SizeAndActionsVec &v =
+            Type.isPointer()
+                ? AddressSpace2SpecifiedActions[Type.getAddressSpace()]
+                : Type.isVector()
+                      ? ElemSize2SpecifiedActions[Type.getElementType()
+                                                      .getSizeInBits()]
+
----------------
t.p.northover wrote:
> 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);
I think both styles are probably roughly equally readable, but I don't have a preference for one over another, so I'll go for your suggestion, thanks!


================
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;
+    }
----------------
t.p.northover wrote:
> I think this is approximately
> 
>     VecIdx = std::lower_bound(Vec.begin(), Vec.end()) - Vec.begin();
> 
> which has added binary-search goodness.
Yeah, I thought of that while writing this, but also thought that typically the Vec array being searched to be very short, and therefore linear search to potentially be faster than binary search. But, true, that is premature optimization not based on any empirical data, and the std::lower_bound expresses the intended semantics more clearly, so I'll look into going with that.


================
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.
----------------
t.p.northover wrote:
> I think the assertion would be reasonable.
I'm still not entirely sure if it wouldn't be possible to come up with a theoretical example where it still would make sense for 2 consecutive actions to both NeedsLegalizingToDifferentSize().
But indeed, probably best to just assert on that and reintroduce the loops if we have an example demonstrating there really is such a case.


https://reviews.llvm.org/D30529