<div dir="ltr">Thank you for interesting discussion!<div><br></div><div>I think you are right and using null pointer is more obvious to represent absence of calculated dominator tree. Patch <a href="https://reviews.llvm.org/D29280">https://reviews.llvm.org/D29280</a> implements this approach.</div></div><div class="gmail_extra"><br clear="all"><div><div class="gmail_signature" data-smartmail="gmail_signature">Thanks,<br>--Serge<br></div></div>
<br><div class="gmail_quote">2017-01-30 19:35 GMT+07:00 Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span class="">On Wed, Jan 25, 2017 at 6:40 PM, Serge Pavlov <span dir="ltr"><<a href="mailto:sepavloff@gmail.com" target="_blank">sepavloff@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span>2017-01-26 0:41 GMT+07:00 Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span>:<br><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 class="gmail_extra"><br><div class="gmail_quote"><span class="m_482136459912265473m_-5790558597981095649gmail-">On Wed, Jan 25, 2017 at 9:16 AM, Serge Pavlov <span dir="ltr"><<a href="mailto:sepavloff@gmail.com" target="_blank">sepavloff@gmail.com</a>></span> wrote:<br><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 class="gmail_extra"><div class="gmail_quote"><span>2017-01-25 23:20 GMT+07:00 Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span>:<br><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">+Chandler.<div><div class="gmail_extra"><br><div class="gmail_quote"><span>On Wed, Jan 25, 2017 at 7:04 AM, Serge Pavlov <span dir="ltr"><<a href="mailto:sepavloff@gmail.com" target="_blank">sepavloff@gmail.com</a>></span> wrote:<br><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>The fact is that sometimes domtree is not available.</div></div></blockquote><div><br></div></span><div>Then why is there a DominatorTree * to use?</div><div>It's fine for the analysis to be not available. When that happens,it should not produce a DominatorTree *.</div><div> <br></div></div></div></div></div></blockquote></span><div>`DominatorTree` is not an anaysis.</div></div></div></div></blockquote><div><br></div></span><div>You are right, DominatorTreeAnalysis or whatever is.</div><div><br></div><div>If we run that analysis, it must return a DominatorTree. That DominatorTree better be valid.</div><div>Otherwise, we done messed up :)</div></div></div></div></blockquote><div><br></div></span><div>Agree. But it is just the case we have, - the analysis was run, no errors found, but it cannot return DominatorTree because it didn't calculate the latter.</div><span><div><br></div></span></div></div></div></blockquote><div><br></div></span><div>FWIW, I am not an expert in the pass manager, but that makes no sense.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><div></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 class="gmail_extra"><div class="gmail_quote"><span class="m_482136459912265473m_-5790558597981095649gmail-"><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 class="gmail_extra"><div class="gmail_quote"><div>`MachineDominatorTree` is an analysis an it is available, `getAnalysis<MachineDominatorT<wbr>ree>()` returns not null.<br></div></div></div></div></blockquote><div><br></div></span><div>Right.</div><div>If that happens, the dominator tree better be valid.</div><div>If it's not going to be valid, we need a good way to signal that in these analysis (currently, both of them use either direct objects or references, sadly) failed, and it needs to be a way that everything can understand, not just "verifyDomTree".</div></div></div></div></blockquote><div><br></div></span><div>This is the root of the problem. We have an analysis that was run without errors but it cannot return its result, it decided that there is no point to run. It is a limitation of the Pass Manager, the latter assumes that if a pass has started, it will return its result or fail. There is no option "result hasn't calculated but it is OK". The patch in <a href="https://reviews.llvm.org/D27190" target="_blank">https://reviews.llvm.org/D2<wbr>7190</a> tried to introduce additional state flag to the `Pass` structure to detect if a pass run successfully but didn't calculated results. With it `getAnalysisIfAvailable` could be used to get pass results. Without such info, we have to deduce if the pass produced its result by indirect evidence. Luckily we have now only one problematic pass.</div></div></div></div></blockquote><div><br></div></span><div>This strategy of "no point to run, but no result available", seems ... concerning.</div><div>But again, i'll leave the viewpoints on the passmanager to chandler.</div><span class=""><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="m_482136459912265473m_-5790558597981095649gmail-"><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 class="gmail_extra"><div class="gmail_quote"><span><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><div class="gmail_extra"><div class="gmail_quote"><div>IE DominatorTree * should be a nullptr you can check against.</div><span><div><br></div></span></div></div></div></div></blockquote></span><div>It can be, not should. `DT` is a private field of `MachineDominatorTree`, there are no requirements how to represent DominatorTree and how to work with it inside the pass. Using the check for presence of roots is merely simpler, the patch is more compact. </div></div></div></div></blockquote><div><br></div></span><div>It's definitely simpler, it just violates all the invariants of the DominatorTree structure :)</div><div>We should avoid that, where, here, we can do that.</div></div></div></div></blockquote><div><br></div></span><div>It cannot violates invariants of something that does not exist :) DominatorTree was not calculated in this case.</div></div></div></div></blockquote><div><br></div></span><div>We're going to agree to disagree here.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="m_482136459912265473m_-5790558597981095649gmail-"><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 class="gmail_extra"><div class="gmail_quote"><span><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><div class="gmail_extra"><div class="gmail_quote"><span><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>If a function is `available_externally` all codegen passes are skipped, although they are marked as required. </div></div></blockquote><div><br></div></span><div>See above :)</div><span><div> </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>MachineDominatorTree is also skipped and there is no domtree at all.</div></div></blockquote><div><br></div></span><div>Again, so why is there a DominatorTree * to use?</div></div></div></div></div></blockquote><div><br></div></span><div>There are no problems to change implementation of `MachineDominatorTree` to use null pointer as indicator of unavailable domtree. It however doesn't present any gain, just another way to fetch info that the pass results are not available.</div><span><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"><div dir="ltr"><div><div class="gmail_extra"><div class="gmail_quote"><span><div> </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> Attempt to call `getRoot` for such domtree will obviously cause a crash. There is no way to get 'right' domtree in this case, so prior to using it we must either:</div><div><span class="m_482136459912265473m_-5790558597981095649gmail-m_3419797503433496657m_-5519904783358674359gmail-m_3348125688270923206m_3336243068098332830m_-2761789139431732640gmail-Apple-tab-span" style="white-space:pre-wrap"> </span>1. Check if corresponding pass was indeed run, or</div><div><span class="m_482136459912265473m_-5790558597981095649gmail-m_3419797503433496657m_-5519904783358674359gmail-m_3348125688270923206m_3336243068098332830m_-2761789139431732640gmail-Apple-tab-span" style="white-space:pre-wrap"> </span>2. Check the domtree state trying to reveal if it is valid.</div><div>The first approach was taken in <a href="https://reviews.llvm.org/D27190" target="_blank">https://reviews.llvm.org/D2719<wbr>0</a> but it was not accepted. This fix tries to use variant 2.</div></div></blockquote><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"><div dir="ltr"><div><br></div><div>Just to emphasize a key point: there are cases when domtree is absent because the pass that it creates was not run and this behavior is by design.</div><div><br></div></div></blockquote><div><br></div></span><div>and in those cases, we should not try to create an invalid datastructure. We should create no datastructure, and let people test against the nullptr.</div><div><br></div></div></div></div></div></blockquote></span><div>We do not create invalid datastructures, </div></div></div></div></blockquote><div><br></div></span><div>I'm not sure how you can argue this. </div><div>The dominator tree you have constructed is not valid. You are then using the invalid datastructure in a different analysis, which happens to know to error out that it's not valid.</div></div></div></div></blockquote><div> </div></span><div>Wrong premise: the dominator tree **was not** calculated. </div></div></div></div></blockquote><div><br></div></span><div>Then the structure should not have been created. Period.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Also the invalid dominator tree is not used by other passes, as they are skipped. But the Pass Manager tries to use it by making call to `verifyDomInfo` in `verifyPreservedAnalysis` and it leads to crash.</div></div></div></div></blockquote><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div>What if i took the result of the analysis, and instead of calling "verify" on it, i called getRoot().</div><div><br>It'll assert, because it's not a valid datastructure.</div></div></div></div></blockquote><div><br></div></span><div>Yes, but nobody tries to use the invalid domtree because all potential users are skipped. It is a problem of pass verification which runs only if expensive checks are enabled. The pass manager doesn't know that the pass was skipped and tries to get its result.</div><span><div> </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 class="gmail_extra"><div class="gmail_quote"><div></div><div>So either we shouldn't have constructed it that way, or we need a way to signal "i ran this thing but it didn't return something valid".</div></div></div></div></blockquote><div><br></div></span><div>This way exactly was implemented in D27190. It was recognized as too complex.</div></div></div></div></blockquote><div><br></div></span><div>Again, i'll leave the pass manager strategy to chandler, but this whole thing smacks me as wrong in a number of ways.</div><div>If it's really that this is a one-off, okay, but otherwise i suspecft we will run into this problem again with something else.</div><span class=""><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><div> </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 class="gmail_extra"><div class="gmail_quote"><div>You should not embed that knowledge that "hey, if this is like this, what it really means is X" in a single function in that datastructure.</div><div><br></div><div>If we really need a way to signal that the analysis ran, but the result is invalid, we should build that, not hack it up.</div></div></div></div></blockquote><div><br></div></span><div>This is a kind of hack, I agree. If it is not acceptable, we need to resurrect D27190.</div></div></div></div></blockquote><div><br></div></span><div>If Chandler truly believes that is not the right way forward, i'd really love to hear his opinion on what should be happening here.</div><div>Assuming he's fine with this one off, i think your #1 below is okay.</div><span class=""><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><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"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class="m_482136459912265473m_-5790558597981095649gmail-"><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 class="gmail_extra"><div class="gmail_quote"><div>`MachineDominatorTree` is a pass and must obey API conventions, `getAnalysis` may be called for it and if it returns nullptr, a user knows that results are absent. `DominatorTree` is an internal datastructure, users don't work with it directly,</div></div></div></div></blockquote><div><br></div></span><div>This is 100% false.</div><div>DominatorTree is a datastructure used *all over the place* directly.</div><div><br></div><div>Users do everything from touching the nodes (at least 300 places) to sorting the children vector to whatever.</div><div><br></div><div>Now, maybe the machine level is different, but even there, this seems quite wrong as a solution to "how do i signal that the analysis result is invalid"</div></div></div></div></blockquote><div><br></div></span><div>Yes, the statement "users don't work with it directly" is wrong. "Users" is also wrong, is this case there is only one user that calls `verifyDomTree', it is the Pass Manager. The idea was that the user should not aware how `verifyDomTree` detects if the pass indeed has produced results.</div><div><br></div><div>To move forward I see several directions:</div><div>1. change the implementation of `MachineDominatorTree` so that it uses null pointer if dominator tree is unavailable,</div><div>2. return to the solution in D27190 and keep flag 'results available' is the Pass state,</div><div>3. something else?</div><div><br></div><div>Do you think case 1 is OK?</div><div><br></div></div></div></div>
</blockquote></span></div><br></div></div>
</blockquote></div><br></div>