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

[Davide Italiano via llvm-commits]

On Wed, Jan 3, 2018 at 2:20 PM, Daniel Sanders via llvm-commits
<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> 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