<html><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Aug 2, 2009, at 4:01 PM, Jakob Stoklund Olesen wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div><br>On 02/08/2009, at 21.47, Evan Cheng wrote:<br><br><blockquote type="cite"><br></blockquote><blockquote type="cite">On Aug 2, 2009, at 6:01 AM, Jakob Stoklund Olesen wrote:<br></blockquote><blockquote type="cite"><blockquote type="cite">* Don't eliminate an identity copy when the source register is<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><undef>. Instead, replace INSERT_SUBREG with an IMPLICIT_DEF<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">instruction.<br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Can we change this to <undef> of the subsequent use(s)? Or is it too<br></blockquote><blockquote type="cite">much work?<br></blockquote><br>That would be preferable, but to be honest, I don't know how to find <br>all the uses. Are we even sure they are in the same MBB? Would I have <br>to modify live-in lists on successor blocks as well?<br><br>LowerExtract() just scans the MBB:<br><br> // Find the kill of the destination register's live range, and <br>insert<br> // a kill of the source register at that point.<br> if (MI->getOperand(1).isKill() && !MI->getOperand(0).isDead())<br> for (MachineBasicBlock::iterator MII =<br> next(MachineBasicBlock::iterator(MI));<br> MII != MBB->end(); ++MII)<br> if (MII->killsRegister(DstReg, &TRI)) {<br> MII->addRegisterKilled(SuperReg, &TRI, /*AddIfNotFound=*/ <br>true);<br> break;<br> }<br><br>Is that correct? DstReg may not be killed in the MBB - it could be <br>live-out. If that happens, Superreg won't be killed either. I don't <br>think that is actually a problem.<br></div></blockquote><div><br></div><div><br></div>Yeah. I don't think it's a good option. Turning it into an IMPLICIT_DEF is perfectly ok. When LiveIntervalAnalysis add <undef> marks on uses of IMPLICIT_DEF, it also leaves those which have uses in other BBs alone.</div><div><br><blockquote type="cite"><div><br><blockquote type="cite"><br></blockquote><blockquote type="cite"><blockquote type="cite">* When replacing %Q1<def, dead> = INSERT_SUBREG ..., add an <imp-<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">kill> of the super register.<br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">I am not sure if I understand this? Can you show me an example?<br></blockquote><br>The test case CodeGen/Blackfin/printf2.ll produces:<br><br><span class="Apple-tab-span" style="white-space:pre"> </span>%R1<def,dead> = INSERT_SUBREG %R1<undef>, %R0L<kill,undef>, 1<br><span class="Apple-tab-span" style="white-space:pre"> </span>ADJCALLSTACKDOWN 12, %SP<imp-def>, %SP<imp-use><br><span class="Apple-tab-span" style="white-space:pre"> </span>CALLa <ga:printf>, %R0<imp-def,dead>, %R1<imp-def,dead>, %R2<imp- <br>def,dead>, ...<br><span class="Apple-tab-span" style="white-space:pre"> </span>ADJCALLSTACKUP 12, 0, %SP<imp-def>, %SP<imp-use><br><span class="Apple-tab-span" style="white-space:pre"> </span>%R0<def> = LOADimm7 0<br><span class="Apple-tab-span" style="white-space:pre"> </span>RTS %R0<imp-use,kill>, %RETS<imp-use><br><br>Undef everywhere. Then we run LowerSubregs:<br><br>subreg: CONVERTING: %R1<def,dead> = INSERT_SUBREG %R1<undef>, <br>%R0L<kill,undef>, 1<br>subreg: %R1L<def,dead> = SLL16i %R0L, 0, %AZ<imp-def>, %AN<imp-def>, <br>%V<imp-def>, %VS<imp-def><br><br>Notice that if %R1 were live, it would be killed by the INSERT_SUBREG <br>but not by the replacement SLL16i. This is a bad example because %R1 <br>is marked <undef>. Pretend that it isn't. LowerInsert already <br>transfers the <dead> flag:<br><br> // Transfer the kill/dead flags, if needed.<br> if (MI->getOperand(0).isDead())<br> TransferDeadFlag(MI, DstSubReg, TRI);<br><br>It just forgets to kill the super-register. I would add %R1<imp- <br>use,kill> to the end of SLL16i to make sure the super-register is dead.<br></div></blockquote><div><br></div>Does it ever happen?</div><div><br><blockquote type="cite"><div><br>This example actually raises another issue:<br><br>%R1<def,dead> = INSERT_SUBREG undef, undef<br><br>can be eliminated, and<br><br>%R1<def> = INSERT_SUBREG undef, undef<br><br>can be replaced with IMPLICIT_DEF. Even when you would normally insert <br>a sub-register copy.<br></div></blockquote><div><br></div>Yes.</div><div><br><blockquote type="cite"><div><br><blockquote type="cite"><br></blockquote><blockquote type="cite"><blockquote type="cite">* Fix a bug in transferring the kill flag from InsReg: InsReg is<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">operand #2, not #1.<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">In the regscavenger:<br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite"><br></blockquote></blockquote><blockquote type="cite"><blockquote type="cite">* Allow imp-use of a dead register.<br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">I don't get this part. Is this using an implicit_def?<br></blockquote><br>It would allow SLL16i ..., %R1<imp-use,kill> even when %R1 is dead. It <br>mirrors the "double define with imp-def is legal" rule.<br></div></blockquote><div><br></div><div>Is this it?</div><span class="Apple-style-span" style="font-family: monospace; "><blockquote type="cite">- assert(isUsed(Reg) && "Using an undefined register!");<br></blockquote><blockquote type="cite">+ assert((MO.isImplicit() || isUsed(Reg)) && "Using an undefined</blockquote><div><br></div><div>But isn't it true you would ever introduce an implicit kill during insert_subreg lowering when the super-register was already live? That means isUsed(Reg) should be true. I really have trouble with this change as it would allow some real bugs through.</div><div><br></div><div>The example you gave is this:</div><div><span class="Apple-style-span" style="font-family: Helvetica; "><blockquote type="cite"><div>%R1<def,dead> = INSERT_SUBREG %R1<undef>, %R0L<kill,undef>, 1</div></blockquote><br></span></div><div><font class="Apple-style-span" face="Helvetica">This should have been translated to an implicit_def. If that's done, do we still have to relax the scavenger?</font></div><div><br></div></span><blockquote type="cite"><div><br><blockquote type="cite"><br></blockquote><blockquote type="cite"><blockquote type="cite">* Allow redefining a sub-register of a live super register.<br></blockquote></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">To be precise, it should be modeled as a use and def. See<br></blockquote><blockquote type="cite">LiveVariables.cpp<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"> // The larger register is previously defined. Now a smaller<br></blockquote><blockquote type="cite">part is<br></blockquote><blockquote type="cite"> // being re-defined. Treat it as read/mod/write if there are<br></blockquote><blockquote type="cite">uses<br></blockquote><blockquote type="cite"> // below.<br></blockquote><blockquote type="cite"> // EAX =<br></blockquote><blockquote type="cite"> // AX = EAX<imp-use,kill>, EAX<imp-def><br></blockquote><br>I see. I will have to update the machine code verifier as well.<br><br>The exact meaning of the kill/def/undef/imp flags is quite hard to get <br>right. Particularly the subreg stuff is difficult. It may be a good <br>idea to document it in great detail, and the make sure the machine <br>code verifier and scavenger agree.<br></div></blockquote><div><br></div>Yes, it's very difficult to model.</div><div><br><blockquote type="cite"><div><br>[...]<br><br><blockquote type="cite"><blockquote type="cite">%Q1<def> = INSERT_SUBREG %Q1<undef>, %D2<kill>, 5<br></blockquote></blockquote><br><blockquote type="cite">Are you saying in this case there isn't a matching q1 = insert_subreg<br></blockquote><blockquote type="cite">%q1, x, 6? I think using implicit_def is perfectly ok then. Part of Q1<br></blockquote><blockquote type="cite">is implicitly defined.<br></blockquote><br>Sometimes there is, sometimes there isn't. Blackfin implements anyext <br>like this:<br><br>def : Pat<(i32 (anyext D16L:$src)),<br> (INSERT_SUBREG (i32 (IMPLICIT_DEF)),<br> (COPY_TO_REGCLASS D16L:$src, D16L),<br> bfin_subreg_lo16)>;<br><br>The high part of the register is left undefined for a true "anyext". <br>When INSERT_SUBREG is lowered, it still must define the full register.<br><br>Example: (i32 (anyext (ONES D:$src))) becomes:<br><br>%R1.L<def> = ONES %R0<kill><br>%R1<def> = INSERT_SUBREG %R1<undef>, %R1.L<kill><br><br>This is perfectly fine until LowerSubregs removes INSERT_SUBREG <br>because it is an identity copy. Then %R1 is no longer live. Actually, <br>it seems a bit dodgy to propagate %R1<undef> to all uses just because <br>it is the result of an "anyext".<br><br><br><blockquote type="cite"><br></blockquote><blockquote type="cite">The patch looks good except for this part:<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">- assert(isUsed(Reg) && "Using an undefined register!");<br></blockquote><blockquote type="cite">+ assert((MO.isImplicit() || isUsed(Reg)) && "Using an undefined<br></blockquote><blockquote type="cite">register!");<br></blockquote><blockquote type="cite"><br></blockquote><blockquote type="cite">Do you mean MO.isUndef()?<br></blockquote><br>No, this is the "Allow imp-use of a dead register" rule discussed <br>above. Perhaps the rule should be "Allow imp-kill of a dead register"?<br><br>assert(((MO.isImplicit() && MO.isKill()) || isUsed(Reg)) && .... ?<br><br>I think we need a way to say "make sure this register is killed, dead <br>or alive". I have been using <imp-kill> for that purpose.<br></div></blockquote><div><br></div>This is discussed in more detail earlier.</div><div><br></div><div>Evan</div><div><br><blockquote type="cite"><div><br>/jakob<br><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br></div></blockquote></div><br></body></html>