[llvm-dev] Glue to connect two nodes in LLVM backend

Tue Jul 16 08:27:06 PDT 2019

Sorry Tim, I clicked on reply instead of reply all.

On Fri, Jul 12, 2019 at 2:15 AM Tim Northover <t.p.northover at gmail.com>
wrote:

> On Thu, 11 Jul 2019 at 18:53, Reshabh Sharma via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
> > 1. Is Gluing the right approach or there is something better for such
> use case?
>
> I would expect the store to take an i64 operand as its address
> (otherwise, in just what sense are your pointers 64-bit?), and
> wouldn't expect to need glue at all.
>

I think we might not need a i64 operand at all (won't they be pain during
the legalization as we are working on RV32), as we plan to introduce new
load/store like NEW_STORE rd rs1 rs2 (where rs1 has lower 32 bits of 64 bit
address and rs2 have higher 32 bits).

>
> There are two key legalization steps here:
>
> 1. Taking an i64:GlobalAddress and converting it into an
> ISD::BUILD_PAIR of two nodes computing the low & high 32-bits of the
> address.
>

The use of BUILD_PAIR sounds really promising (I was considering to use a
custom wrapper node to do the same thing). I took some time to try to make
it work but I guess the legalizer fails after seeing a node with result
type i64. I tried to be sure about it but it does not fail when it tries to
legalize the BUILD_PAIR node with return type i64 instead it fails when it
try to legalize the store node having this as an operand.

When I tried to replace BUILD_PAIR with a WRAPPER node which does the same
thing but had a return type MVT::Glue (I was not sure so just used this,
MVT::Other might be a better option?). Something like this,
t16: i32 = ADDI t15, ...
t20: i32 = ADDI t19, ...
t21: glue = RISCVISD::WRAPPER t16, t20
This passed the legalizer and failed at the instruction selection (which it
should).

What do you think about it? I think the reason for failure (using
BUILD_PAIR) is the i64 result which is not legal so the codegen tries to
expand it over store, in which it fails as it does not know how to do that.

2. Whatever replaces the store splitting its 64-bit pointer operand
> into 2 32-bit values (via the inverse, ISD::EXTRACT_ELEMENT).
>
> The combiner will eliminate the extra nodes from these steps, leading
> to a single customized store taking two well-defined 32-bit operands.
> All values would be tracked directly, and not need glue to convey a
> side-channel of information. I think trying to use glue is misguided.
>

Yes, now I agree that this is a far better way to deal with it.

>
> > 2. If I do something like DAG.getNode(ISD::ADDI, DL, MVT::Glue, LUINode,
> OtherADDINode). How will it know that the other ADDI node should be glued
> and not LUI. I seriously feel I'm missing something.
>
> Again, I don't think Glue applies here, but I will try to describe how
> it *is* used elsewhere a little...
>
> If two nodes need glueing, that information is conveyed via a special
> extra operand of type MVT::Glue. Some nodes produce this in addition
> to their real outputs, and others consume it in addition to their real
> inputs. Implementation-wise in your case you'd:
>
> 1. Create the OtherADDI node with an extra[*] output type MVT::Glue
> beyond what would be expected (it should be the last output type).
> 2. Add that particular edge as an extra input (aside from the normal
> inputs) to the ADDI you're creating now. If it's a duplicate, that
> probably means you didn't need the glue in the first place because
> normal dependency tracking would handle the situation. Otherwise, your
> node now has an extra MVT::Glue edge that tracks this hidden
> dependency. The glue should also be the last operand.
>
> You also have to declare this kind of relationship in TableGen if
> you're planning to use these nodes there, via SDNPInGlue and
> SDNPOutGlue.
>
>
Million thanks Tim, I was not aware about quite a many things. I'm happy I
got to know new things. The BUILD_PAIR approach looks a lot cleaner than
the Glue one.

On Fri, Jul 12, 2019 at 2:15 AM Tim Northover <t.p.northover at gmail.com>
wrote:

> On Thu, 11 Jul 2019 at 18:53, Reshabh Sharma via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
> > 1. Is Gluing the right approach or there is something better for such
> use case?
>
> I would expect the store to take an i64 operand as its address
> (otherwise, in just what sense are your pointers 64-bit?), and
> wouldn't expect to need glue at all.
>
> There are two key legalization steps here:
>
> 1. Taking an i64:GlobalAddress and converting it into an
> ISD::BUILD_PAIR of two nodes computing the low & high 32-bits of the
> address.
> 2. Whatever replaces the store splitting its 64-bit pointer operand
> into 2 32-bit values (via the inverse, ISD::EXTRACT_ELEMENT).
>
> The combiner will eliminate the extra nodes from these steps, leading
> to a single customized store taking two well-defined 32-bit operands.
> All values would be tracked directly, and not need glue to convey a
> side-channel of information. I think trying to use glue is misguided.
>
> > 2. If I do something like DAG.getNode(ISD::ADDI, DL, MVT::Glue, LUINode,
> OtherADDINode). How will it know that the other ADDI node should be glued
> and not LUI. I seriously feel I'm missing something.
>
> Again, I don't think Glue applies here, but I will try to describe how
> it *is* used elsewhere a little...
>
> If two nodes need glueing, that information is conveyed via a special
> extra operand of type MVT::Glue. Some nodes produce this in addition
> to their real outputs, and others consume it in addition to their real
> inputs. Implementation-wise in your case you'd:
>
> 1. Create the OtherADDI node with an extra[*] output type MVT::Glue
> beyond what would be expected (it should be the last output type).
> 2. Add that particular edge as an extra input (aside from the normal
> inputs) to the ADDI you're creating now. If it's a duplicate, that
> probably means you didn't need the glue in the first place because
> normal dependency tracking would handle the situation. Otherwise, your
> node now has an extra MVT::Glue edge that tracks this hidden
> dependency. The glue should also be the last operand.
>
> You also have to declare this kind of relationship in TableGen if
> you're planning to use these nodes there, via SDNPInGlue and
> SDNPOutGlue.
>
> Cheers.
>
> Tim.
>
> [*] I wouldn't be surprised if this meant you needed a new
> ISD::ADDIWithGlue instead. I haven't done it in years, but you should
> be prepared in case a single node with two different output profiles
> is more than LLVM can cope with.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20190716/d4efefb3/attachment.html>