[PATCH] D30529: [GlobalISel] Enable specifying how to legalize non-power-of-2 size types.

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added a comment.

>> I hope the above makes sense?
> 
> It does, but there are two issues here:

Those are probably the most interesting questions around this patch that I don't have an answer too, and I hope this review can help with getting closer to an answer.
Thanks for making them explicit here. (Of course there may be other big issues hiding here, but I'm not aware of them at the moment).

The hard part is that I don't think there's a good answer yet indeed on the question on whether it is possible to incrementally specify how to legalize different bitsizes on a specific operation.
A few more thoughts inline below.

> 1. How would this inter-operate with table-gen?
> 
>   IIUC, the idea is to move as much as possible to table-gen. Currently (SelDAG), instructions that are described in table-gen are "legal". It would be good to re-use as much as possible of that, to avoid table-gen bloat.
> 
>   Are we going to ignore that info and re-build a specific lowering database? Or re-use that for the lowering (thus needing merge, see below)? Or is this technique only for when the generic instruction doesn't map to anything in table-gen?

Indeed, it would be best not to ignore that info. Or at least not violate the DRY principle. Or if we did end up violating that principle, in an asserts-build make sure that we'd assert if the different pieces of info would conflict.
That being said, there might be a hint of a solution already in this patch. One of the helper functions to more concisely specify how to legalize all bit sizes in this patch is `getWidenToLargerTypesAndNarrowToLargest`.
Let me copy paste the documentation I wrote for it here:

  /// Helper function for the common case where legalization for a particular
  /// operation consists of widening the type to a large legal type, unless
  /// there is no such type and then instead it should be narrowed to the
  /// largest legal type. E.g.
  /// setAction({G_ADD, LLT:scalar(1)},
  ///           {{1, WidenScalar},  // bit sizes [ 1, 31[
  ///            {32, Legal},       // bit sizes [32, 33[
  ///            {33, WidenScalar}, // bit sizes [33, 64[
  ///            {64, Legal},       // bit sizes [64, 65[
  ///            {65, NarrowScalar} // bit sizes [65, +inf[
  ///           });
  /// can be shortened to:
  /// setAction({G_ADD, LLT:scalar(1)},
  ///           getWidenToLargerTypesAndNarrowToLargest(
  ///            {32, Legal}, {64, Legal}));

The info that a G_ADD is legal on 32 and 64-bit types could indeed be retrieved from tablegen.
The fact that WidenScalar is a good way to legalize if there is a wider legal type is, if I'm not mistaken, target-indepedent, so that could be logic in the target-independent part.
I'm not entirely sure on the exact conditions for NarrowScalar to be an appropriate way to legalize adds that are larger than the largest legal size. Maybe it is also fully target-independent.
If it is, than the above setAction could indeed fully be derived from tablegen info and some target-independent logic.
FWIW, the above seems quite similar to what current SelDAG type-legalization does at https://github.com/llvm-mirror/llvm/blob/master/lib/CodeGen/TargetLoweringBase.cpp#L1341 (if I understand that code correctly).

> 2. Would this allow merging data?
> 
>   When sub-arch specific decisions are concerned, having a way to override a base default case would reduce the amount of code on both table-gen and c++ parts.
> 
>   For example, we could have a default catch-all case like `{1,widen; 32,legal; 33,unsupp}`, and later on, based on sub-arch decisions, increment legality, lib calls, etc.

With the patch as is, you indeed need to re-specify the info for all bit sizes.
It could be that I pushed the current patch way too far in breaking existing APIs and if I turned back the patch to only change the internal representations used in LegalizerInfo.cpp,
this would work. So, the idea being that tablegen info and targets specify for which data types an action can be taken that doesn't require changing bitsize, such as Legal, Lower, Libcall.
And then the target-independent logic can hopefully make decisions on most/all operations on how to adapt types to different bitsizes when that's needed.
I'll look into that.

Thanks for sharing your thoughts!


https://reviews.llvm.org/D30529