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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

I haven't read the actual code yet, but I've got a couple questions and a comment based on the description and the conversation so far.

> Another major change is that getAction no longer returns a single action, but
>  returns a sequence of actions, as legalizing non-power-of-2 types may need
>  multiple actions. For example: findLegalAction({G_REM, 13}) should return
> 
> [(WidenScalar, 32), (Lower, 32)], indicating to first widen the s13
>  scalar to 32 bits, and to then lower it, assuming the setAction on SREM
>  was something like:
>  setAction({G_REM, LLT:scalar(1)},
> 
>   {{1, WidenScalar},  // bit sizes [ 1, 31[
>    {32, Lower},       // bit sizes [32, 33[
>    {33, NarrowScalar} // bit sizes [65, +inf[
>   });

Does findLegalAction() need to return a sequence here? I'm thinking that it could simply be called twice:

  iterator I = findLegalAction({G_REM, 13}); // *I == (WidenScalar, 32)
  iterator J = findLegalAction({G_REM, 32}); // *J == (Lower, 32), I could also be given as an argument to speed up the search

Also, given that the 2nd argument to setAction() describes all the bit sizes, is the bit-size of the LLT::scalar(1) still required for something?

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

I agree that there's a correlation there but I don't think it's the tablegen definition that specifies that they are legal. In SelectionDAG, it's the calls to setOperationAction() that specify legality and the default is 'Legal'.

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

One way to allow mergable data in your current API is with an 'Inherit' action like so:

  setAction({G_ADD, LLT::scalar(1)}, {{1, Inherit}, {33, WidenScalar}, {64, Legal}, {65, NarrowScalar}});

This would keep existing actions for sizes 1-32, and replace the actions for size 33 and up.

One other possibility is to move the specification to tablegen, and have it figure out an array layout that is cheap to configure. For example, it could decide to take

  {{1, WidenScalar}, {32, Legal}, {33, NarrowScalar}
  {{1, WidenScalar}, {32, Legal}, {33, WidenScalar}, {64, Legal}, {65, NarrowScalar}} // if 64-bit supported

and create a default array like so:

  {{1, WidenScalar}, {32, Legal}, {33, NarrowScalar}, {64, NarrowScalar}, {65, NarrowScalar}}

so that when 64-bit support is enabled it can just replace elements 2 and 3 with:

  {33, WidenScalar}, {64, Legal}


https://reviews.llvm.org/D30529