<div dir="ltr"><div dir="ltr"><span class="gmail-im"><div class="gmail_attr">Sorry Tim, I clicked on reply instead of reply all.</div><div dir="ltr" class="gmail_attr"><br></div><div dir="ltr" class="gmail_attr">On Fri, Jul 12, 2019 at 2:15 AM Tim Northover <<a href="mailto:t.p.northover@gmail.com" target="_blank">t.p.northover@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Thu, 11 Jul 2019 at 18:53, Reshabh Sharma via llvm-dev<br><<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br>> 1. Is Gluing the right approach or there is something better for such use case?<br><br>I would expect the store to take an i64 operand as its address<br>(otherwise, in just what sense are your pointers 64-bit?), and<br>wouldn't expect to need glue at all.<br></blockquote><div><br></div></span><div>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).   <br></div><span class="gmail-im"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>There are two key legalization steps here:<br><br>1. Taking an i64:GlobalAddress and converting it into an<br>ISD::BUILD_PAIR of two nodes computing the low & high 32-bits of the<br>address.<br></blockquote><div><br></div></span><div>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. </div><div><br></div><div>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,</div><div>t16: i32 = ADDI t15, ...<br></div><div>t20: i32 = ADDI t19, ...<br></div><div>t21: glue = RISCVISD::WRAPPER t16, t20<br></div><div>This passed the legalizer and failed at the instruction selection (which it should).</div><div><br></div><div>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. <br></div><span class="gmail-im"><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">2. Whatever replaces the store splitting its 64-bit pointer operand<br>into 2 32-bit values (via the inverse, ISD::EXTRACT_ELEMENT).<br><br>The combiner will eliminate the extra nodes from these steps, leading<br>to a single customized store taking two well-defined 32-bit operands.<br>All values would be tracked directly, and not need glue to convey a<br>side-channel of information. I think trying to use glue is misguided.<br></blockquote><div><br></div></span><div>Yes, now I agree that this is a far better way to deal with it. </div><span class="gmail-im"><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>> 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.<br><br>Again, I don't think Glue applies here, but I will try to describe how<br>it *is* used elsewhere a little...<br><br>If two nodes need glueing, that information is conveyed via a special<br>extra operand of type MVT::Glue. Some nodes produce this in addition<br>to their real outputs, and others consume it in addition to their real<br>inputs. Implementation-wise in your case you'd:<br><br>1. Create the OtherADDI node with an extra[*] output type MVT::Glue<br>beyond what would be expected (it should be the last output type).<br>2. Add that particular edge as an extra input (aside from the normal<br>inputs) to the ADDI you're creating now. If it's a duplicate, that<br>probably means you didn't need the glue in the first place because<br>normal dependency tracking would handle the situation. Otherwise, your<br>node now has an extra MVT::Glue edge that tracks this hidden<br>dependency. The glue should also be the last operand.<br><br>You also have to declare this kind of relationship in TableGen if<br>you're planning to use these nodes there, via SDNPInGlue and<br>SDNPOutGlue.<br><br></blockquote><div> </div></span><div>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. </div></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Fri, Jul 12, 2019 at 2:15 AM Tim Northover <<a href="mailto:t.p.northover@gmail.com">t.p.northover@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Thu, 11 Jul 2019 at 18:53, Reshabh Sharma via llvm-dev<br>
<<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>> wrote:<br>
> 1. Is Gluing the right approach or there is something better for such use case?<br>
<br>
I would expect the store to take an i64 operand as its address<br>
(otherwise, in just what sense are your pointers 64-bit?), and<br>
wouldn't expect to need glue at all.<br>
<br>
There are two key legalization steps here:<br>
<br>
1. Taking an i64:GlobalAddress and converting it into an<br>
ISD::BUILD_PAIR of two nodes computing the low & high 32-bits of the<br>
address.<br>
2. Whatever replaces the store splitting its 64-bit pointer operand<br>
into 2 32-bit values (via the inverse, ISD::EXTRACT_ELEMENT).<br>
<br>
The combiner will eliminate the extra nodes from these steps, leading<br>
to a single customized store taking two well-defined 32-bit operands.<br>
All values would be tracked directly, and not need glue to convey a<br>
side-channel of information. I think trying to use glue is misguided.<br>
<br>
> 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.<br>
<br>
Again, I don't think Glue applies here, but I will try to describe how<br>
it *is* used elsewhere a little...<br>
<br>
If two nodes need glueing, that information is conveyed via a special<br>
extra operand of type MVT::Glue. Some nodes produce this in addition<br>
to their real outputs, and others consume it in addition to their real<br>
inputs. Implementation-wise in your case you'd:<br>
<br>
1. Create the OtherADDI node with an extra[*] output type MVT::Glue<br>
beyond what would be expected (it should be the last output type).<br>
2. Add that particular edge as an extra input (aside from the normal<br>
inputs) to the ADDI you're creating now. If it's a duplicate, that<br>
probably means you didn't need the glue in the first place because<br>
normal dependency tracking would handle the situation. Otherwise, your<br>
node now has an extra MVT::Glue edge that tracks this hidden<br>
dependency. The glue should also be the last operand.<br>
<br>
You also have to declare this kind of relationship in TableGen if<br>
you're planning to use these nodes there, via SDNPInGlue and<br>
SDNPOutGlue.<br>
<br>
Cheers.<br>
<br>
Tim.<br>
<br>
[*] I wouldn't be surprised if this meant you needed a new<br>
ISD::ADDIWithGlue instead. I haven't done it in years, but you should<br>
be prepared in case a single node with two different output profiles<br>
is more than LLVM can cope with.<br>
</blockquote></div>