<div dir="ltr"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jul 22, 2019 at 10:28 PM Reshabh Sharma <<a href="mailto:reshabhsh@gmail.com">reshabhsh@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"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div>Million thanks Tim, your replies always take us to a better and much cleaner approach.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Jul 17, 2019 at 12:47 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 Tue, 16 Jul 2019 at 16:24, Reshabh Sharma <<a href="mailto:reshabhsh@gmail.com" target="_blank">reshabhsh@gmail.com</a>> wrote:<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>
> 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>
<br>
That makes sense, and should interact well with the pair/extract idiom<br>
(with some minor annoyance to create those stores taking two sources).<br>
<br>
> 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.<br>
<br>
> 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>
<br>
If it's not feeding directly into an EXTRACT of the usual i64 -><br>
(i32,i32) type legalization[*] kind (I think EXTRACT_ELEMENT) then I<br>
would expect failure. So you need to replace your store with the<br>
custom RS1, RS2 variant at the type legalization phase. That's very<br>
early, and might not even have hooks. If you're finding that then the<br>
first DAG-combine phase runs before type legalization, and definitely<br>
can be hooked to replace all stores with your custom node.<br>
<br></blockquote><div>Yeah this seems far better, we should be doing this already. Last week (after reading your reply) I tried to do this at the type legalization phase. I could successfully replace store node with the custom store (but things didn't go well).</div><div>I am replacing the following store node:</div><div><font face="courier new, monospace"><br></font></div><div><font face="courier new, monospace">ch = store<(store 4 into `i32 addrspace(1)* getelementptr inbounds ([1 x i32], [1 x i32] addrspace(1)* @foo, i32 0, i32 0)`, addrspace 1)> t4, Constant:i32<87>, GlobalAddress:i64<[1 x i32] addrspace(1)* @foo> 0, undef:i64<br></font></div><div><font face="courier new, monospace"><br></font></div><div><font face="courier new, monospace">where,</font></div><div><font face="courier new, monospace">t4 : ch = store<(store 4 into %ir.retval)> t0, Constant:i32<0>, FrameIndex:i32<0>, undef:i32</font></div><div><font face="courier new, monospace">t0 : ch = EntryToken</font></div><div><br></div><div>into:</div><div><br></div><div>ch = CUSTOM_STORE <span style="font-family:"courier new",monospace">Constant:i32<87>, tx, ty, t4</span></div><div><span style="font-family:"courier new",monospace"><br></span></div><div><span style="font-family:"courier new",monospace">where tx and ty are ADDI for Hi and Lo. </span></div><div><span style="font-family:"courier new",monospace"><br></span></div><div><font face="arial, sans-serif">If I don't keep t4 in the operand list, it fails at second DAG combiner phase, saying entry node not found. I found your reply on a thread about chaining loads and stores(<a href="http://lists.llvm.org/pipermail/llvm-dev/2017-June/114783.html" target="_blank">http://lists.llvm.org/pipermail/llvm-dev/2017-June/114783.html</a></font><span style="font-family:arial,sans-serif">) and it makes some sense to me that we need to have the custom store chained as it was before(?) and when I tried that, it failed an assertion at the end of scheduler, saying "</span><font face="arial, sans-serif">#operands for dag node doesn't match .td file!" (This makes some sense to me, as per .td file it should have three operand not four(?)). Can you please suggest something for this, I don't have any backup plan to make this work (I'm stuck).</font></div><div><font face="arial, sans-serif"><br></font></div><div><font face="arial, sans-serif">SelectionDAG after instruction selection (Line 11 has the CUSTOM_STORE)</font></div><div><ol><li> t0: ch = EntryToken<br></li><li> t5: i32 = ADDI Register:i32 $x0, TargetConstant:i32<4><br></li><li> t34: i32,ch = CopyFromReg t0, Register:i32 $x0<br></li><li> t4: ch = SW<Mem:(store 4 into %ir.retval)> t34, TargetFrameIndex:i32<0>, TargetConstant:i32<0>, t0<br></li><li> t7: ch = SW<Mem:(store 4 into %ir.a)> t5, TargetFrameIndex:i32<1>, TargetConstant:i32<0>, t4<br></li><li> t8: i32 = ADDI Register:i32 $x0, TargetConstant:i32<87><br></li><li> t19: i32 = LUI TargetGlobalAddress:i32<[1 x i32] addrspace(1)* @foo> 0 [TF=2]<br></li><li> t20: i32 = ADDI t19, TargetGlobalAddress:i32<[1 x i32] addrspace(1)* @foo> 0 [TF=3]<br></li><li> t23: i32 = LUI TargetGlobalAddress:i32<[1 x i32] addrspace(1)* @foo> 0 [TF=4]<br></li><li> t24: i32 = ADDI t23, TargetGlobalAddress:i32<[1 x i32] addrspace(1)* @foo> 0 [TF=1]<br></li><li> t27: ch = CUSTOM_STORE t8, t20, t24, t7<br></li><li> t13: i32,ch = LW<Mem:(dereferenceable load 4 from %ir.a)> TargetFrameIndex:i32<1>, TargetConstant:i32<0>, t27<br></li><li> t15: ch,glue = CopyToReg t27, Register:i32 $x10, t13<br></li><li> t16: ch = PseudoRET Register:i32 $x10, t15, t15:1<br></li></ol></div><div><br></div></div></div></div></div></div></div></div></div></div></blockquote><div><br></div><div>We were using a wrong input as chain input, I've fixed it and we could pass this. We are now failing at instruction printer and I think the reason is the missing value operand in our custom store after the conversion to machine instruction (More description: <a href="http://lists.llvm.org/pipermail/llvm-dev/2019-July/134119.html">http://lists.llvm.org/pipermail/llvm-dev/2019-July/134119.html</a>)</div><div><br></div><div>Many thanks, </div><div>Reshabh</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div class="gmail_quote"><div></div><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">
Cheers.<br>
<br>
Tim.<br>
<br>
[*] In case this term isn't familiar, the sequence of DAG phases goes<br>
something like:<br>
<br>
Combine 1 -> Type legalization -> Combine 2 -> Legalization -> Selection<br>
<br>
After type legalization (for you) *nothing* should have type i64, so<br>
if you need to replace things you have to do it before then. I believe<br>
a part of type legalization will fold BUILD_PAIR/EXTRACT_ELEMENT pairs<br>
to eliminate incidental/trivial i64s.<br></blockquote><div><br></div><div>Many thanks</div><div>Reshabh </div></div></div></div></div></div></div></div></div></div>
</blockquote></div></div></div>