[llvm] r321653 - [BasicBlockUtils] Check for unreachable preds before updating LI in UpdateAnalysisInformation

[Anna Thomas via llvm-commits]

Added assert in https://reviews.llvm.org/rL321805

Thanks
Anna
On Jan 4, 2018, at 10:48 AM, Davide Italiano <davide at freebsd.org<mailto:davide at freebsd.org>> wrote:

On Wed, Jan 3, 2018 at 2:20 PM, Daniel Sanders via llvm-commits
<llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>> wrote:
Hi Philip,

On 3 Jan 2018, at 13:31, Philip Reames via llvm-commits
<llvm-commits at lists.llvm.org<mailto:llvm-commits at lists.llvm.org>> wrote:

Volkan,

If you are requiring loop info, you are computing domtree.  You can add a
requirement for domtree and pass it in at no additional runtime cost.


I agree, I'm not sure why it's currently optional in our downstream pass but
this would certainly fix the problem.

Moreover, Anna has made a perfectly reasonable change.  We do not restrict
upstream development to prevent breaking downstream work.  We're happy to
help guide you in migrating your downstream code, but you should not expect
that downstream breakage (even if it wasn't easy to fix) would be a cause
for an upstream revert.  It is not.


I don't believe Volkan requested that the patch be reverted so I'm not sure
where the harsh tone is coming from. Is there something I've missed?

As far as I can see, there's a belief that
addRequired<LoopInfoWrapperPass>() ensures that the domtree is available but
while that's often the case it isn't quite guaranteed as the pass execution
log Volkan provided shows. That said, calling UpdateAnalysisInformation()
without having a domtree to update is strange at best (the comment says it
updates the domtree and you can't update something you don't have) and as
Anna explained you need the domtree to update the loop info anyway.

I currently think that we should something like:
assert(DT && "DomTree must be available to update loop info");
to the code so that passes that update the loop info but didn't ensure the
dom tree is available with their own addRequired<DomTreeWrapperPass>() get a
better hint as to what they need to do.


Sorry, I was traveling and I lost this. As reviewer of the previous
change, I agree adding the assertion is a good thing.
The change is correct, but it's generally better to give people "fail
early fail loud" messages rather than silent failures.
I don't fancy the idea of a reference as it causes a lot of issues
with invalidation etc..

Thanks,

--
Davide

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180104/9bcc02d3/attachment.html>