<div dir="ltr"><div class="gmail_quote"><div dir="ltr">On Mon, May 1, 2017 at 11:34 PM Sanjoy Das <<a href="mailto:sanjoy@playingwithpointers.com">sanjoy@playingwithpointers.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi,<br>
<br>
On Mon, May 1, 2017 at 10:18 AM, Chandler Carruth <<a href="mailto:chandlerc@google.com" target="_blank">chandlerc@google.com</a>> wrote:<br>
>> Also, IIUC, today deleting an edge from A to B only requires<br>
>> removeIncomingValue(A) on B's PHI nodes, but after this change you'll<br>
>> have to check if you're deleting the last edge or not.<br>
><br>
> Are there many places where this is a feature rather than a bug though?<br>
><br>
> I didn't do a survey of every caller of removeIncomingValue, but the 4 or 5<br>
> I looked at were all inside of a loop already. Which actually means they<br>
> would get more efficient with this change, if in some cases needing to keep<br>
> a set around.<br>
><br>
> This doesn't seem too bad to me, but again, maybe I'm looking at the wrong<br>
> bits of code. Is there specific code you have in mind that wants to remove<br>
> one edge (but not one predecessor block)?<br>
<br>
I think most calls to BasicBlock::removePredecessor would need<br>
auditing.  For instance, in llvm::ConstantFoldTerminator when we<br>
transform:<br>
<br>
BB:<br>
  br true, A, B<br>
<br>
to<br>
<br>
BB:<br>
  br A<br>
<br>
then we only need to call B->removePredecessor(BB) which, in turn,<br>
only needs to PhiInB->removeIncomingValue(BB).  With your<br>
representation we'll have to worry if A == B.<br></blockquote><div><br></div><div>I guess I expect that after B->removePredecessor(BB), BB is *not* a predecessor any more.</div><div><br></div><div>I guess you're saying that a 'predecessor' is really an 'edge'. Ok, if confusing to me.</div><div><br></div><div>Still, wouldn't we just need to audit the *implementation* of removePredecessor? It should hide all of this. My real point was that directly manipulating the PHI node seems to be rare.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">>> Can (2), (3) and (4) be fixed by changing the API instead of the<br>
>> deeper IR change you're proposing?<br>
><br>
> Maybe, but I don't see easy ways. Ideas?<br>
<br>
For (2), will it help if there was a setIncomingValueForBlock(BB, V)<br>
that DTRT with setting (all) the incoming value(s) for BB to V?<br>
<br>
For (4) perhaps we could add a removeAllIncomingValuesForBlock helper?<br></blockquote><div><br></div><div>These, IMO, turn the PHI incoming values into an inefficient set. ;] We'll still be reasoning about whether we're decreasing the count of edges to zero or not, but we'll be storing an incoming value and a block for each edge instead of just counting the edges that are already there.</div><div><br></div><div>Somewhat worse, this duplication is magnified by the number of PHI nodes.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
I'm not sure what exactly you meant by (3).<br></blockquote><div><br></div><div>If I want to change the incoming value for one *edge*, but not for others, I find it confusing that I just find any old incoming value for that edge and remove it. It just seems odd that we keep N copies, but they don't have any semantic beyond the count of them being N. We never check which edge we flow in through to pick which of the N copies to use because we insist that they're all the same.</div><div><br></div><div>Anyways, this isn't causing any immediate pain, I just wanted to understand why we're using what seems like a very confusing representation to me.</div></div></div>