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

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls marked 9 inline comments as done.
kristof.beyls added inline comments.


================
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(
----------------
kristof.beyls wrote:
> 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.
I've made G_ADD widenToLargerTypesAndNarrowToLargest.
There are going to be lots of tiny functional differences in here that will be extremely hard to avoid, so I might as well change this.


================
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;
+    }
----------------
kristof.beyls wrote:
> 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.
It turned out to be slightly less trivial than VecIdx = std::lower_bound(...); but still concise enough to go for it.


================
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.
----------------
kristof.beyls wrote:
> 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.
It turns out the loops are actually needed - see new comment I put in in case NarrowScalar to explain why.


================
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);
----------------
t.p.northover wrote:
> I think this is the same as `widen_1_8_16_narrowToLargest` isn't it?
Good catch! I removed the function and replaced its uses with `wide_1_8_16_narrowToLargest`


https://reviews.llvm.org/D30529