[PATCH] D62423: [globalisel][legalizer] Attempt to write down the minimal legalization rules

Mon Jun 17 13:27:12 PDT 2019

dsanders marked 2 inline comments as done.
dsanders added a comment.

In D62423#1517863 <https://reviews.llvm.org/D62423#1517863>, @Petar.Avramovic wrote:

> Producer/Consumer type set and minimum rules for scalars are assuming that set of available integer scalars is the same as set of all available floating point scalars. This does not have to be the case.

Scalars are a bag of bits that do not have an associated floating-point/integer interpretation. That interpretation is provided by the operation

> Could we define Producer/Consumer type set in terms of available sizes in register banks?
>  That is Producer/Consumer type set for G_ANYEXT, G_SEXT, G_ZEXT, G_TRUNC corresponds to available sizes in GPR register bank (integer instructions). 
>  That is Producer/Consumer type set for G_FPEXT, G_FPTRUNC corresponds to available sizes in FPR register bank (floating point instructions).

No, because register banks aren't a concept that legalization operates on. They're mostly introduced by the Register Bank Allocator

> This is the problem with GMIR in general since type (integer/float) is not available in low level type, but in most cases it is possible to figure out the type from opcode (float opcodes start with G_F , integer start with G_).

You shouldn't be trying to figure out the data format of a scalar in GMIR as you'll get contradictory results (e.g. G_ADD feeding into G_FADD). The operations impart an interpretation on the bits in a register w.r.t that operation but the values in registers are just abstract bundles of bits.

> There are also ambiguous opcodes that are used for both integer and floating point instructions assuming they are available for same sizes (G_PHI, G_LOAD, G_STORE, G_IMPLICIT_DEF).

They're not really ambiguous, the data format just has no meaning to them. G_PHI/G_LOAD/G_STORE are just moving bundles of bits around without interpreting them. G_IMPLICIT_DEF just invents a bundle of bits with unspecified values.

In D62423#1518267 <https://reviews.llvm.org/D62423#1518267>, @qcolombet wrote:

> Hi Daniel,
>
> Thanks for writing this down. I think the description is accurate (modulo the bit cast stuff) of what we do today.
>  That said, the vision (that we don't implement currently) for the legalizer was actually much simpler:
>
> - The target needs to provide a way to legalize exts from producer/consumer sets.
> - The target needs to support loading and storing from producer/consumer sets.
>
>   From there, supporting truncates, merge/unmerge, build vector and so on is a matter of issuing the right loads and stores. That's not very efficient thus we encourage targets to do what you wrote down, but the idea was that it is more an optimization than anything else.

Those rules work when there's a load/store for every type but they run into unable-to-legalize if that's not the case. For example, suppose you have 8, 16, and 32-bit load/store but you also have 64-bit add. The s64 would need to narrowScalar to be usable with the s32 load/stores but that's not possible if narrowScalar needs an s64 store to narrow it. You'd need either G_TRUNC + G_LSHR (the G_LSHR can be a libcall) or G_UNMERGE_VALUES (which itself can legalize to G_TRUNC+G_LSHR) to legalize that case.

================
Comment at: llvm/docs/GlobalISel.rst:422
+
+Minimum Rules For Scalars
+"""""""""""""""""""""""""
----------------
Petar.Avramovic wrote:
> Here we are assuming that set of available integer scalars is the same as set of all available floating point scalars. This does not have to be the case. 
> e.g. "-mtriple=mipsel-linux-gnu" "-mtriple=i386-linux-gnu -mattr=+sse2"(http://lists.llvm.org/pipermail/llvm-dev/2017-July/114930.html) have i32, float/double available but i64 is not available.
> G_FPTRUNC is legal for {s32, s64} but G_TRUNC is not (for -mtriple=mipsel-linux-gnu I would say that it is unsupported since there are no register classes that hold less then 32 bits, and would expect artifact combiner to deal with G_TRUNC).
> 
> Here we are assuming that set of available integer scalars is the same as set of all available floating point scalars. This does not have to be the case. 
> e.g. "-mtriple=mipsel-linux-gnu" "-mtriple=i386-linux-gnu -mattr=+sse2"(http://lists.llvm.org/pipermail/llvm-dev/2017-July/114930.html) have i32,
> float/double available but i64 is not available.

No, the interpretation of the bits as being integer or floating point is irrelevant to this. An s64 containing a floating point value is still required to be truncatable with G_TRUNC. For example, this is necessary to feed it into an s32 store instruction. Similarly, an s32 containing half of a floating point value is still required to be any-extendable with G_ANYEXT in order to compose it with the other half to construct a full value.

> G_FPTRUNC is legal for {s32, s64} but G_TRUNC is not (for -mtriple=mipsel-linux-gnu I would say that it is unsupported since there are no register > classes that hold less then 32 bits, and would expect artifact combiner to deal with G_TRUNC).

G_TRUNC must be legal from s64 to s32 for a target to be guaranteed a means to store the s64 to memory (MIPS would implement it using mfc1). You're unlikely to actually use that path since MIPS you also have legal operations that make the guaranteed path redundant (e.g. sdc1)

================
Comment at: llvm/docs/GlobalISel.rst:455
+only operation to account for. We don't require that it's legal because it
+can be lowered to COPY (or to nothing using replaceAllUses()).
+
----------------
qcolombet wrote:
> Just a remark.
> Bitcasts can actually be tricky to lower  (from langref: `The conversion is done as if the value had been stored to memory and read back as type ty2`) because it can shuffle bits around.
> Therefore we should add that if the bistcast for the dst and src type is not a no-op, then the target should know how to expand it.
Yep, I should clarify that. The intent was that because some targets can cope without it there's no requirement in the minimal set. I'll update the wording on this shortly

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D62423/new/

https://reviews.llvm.org/D62423