[PATCH] D155528: [SimplifyCFG][FIX] Update GlobalsAA after an assumption was created

Tue Jul 18 08:58:59 PDT 2023

jdoerfert abandoned this revision.
jdoerfert added a comment.

In D155528#4509210 <https://reviews.llvm.org/D155528#4509210>, @nikic wrote:

> Let's look at @aeubanks case first. We have a transform like this:
>
> I believe this transform is correct.

I think I agree with you.

> -----
>
> And if yes, then I think it's clear that the approach you're persuing here cannot work. This means that we have to update the call graph every time an intrinsic call is introduced, while we currently widely assume that this is not necessary. In D141190 <https://reviews.llvm.org/D141190> you implemented a change to start including non-nocallback intrinsics in the call graph and made GlobalsAA rely on that -- I think we just found out what the reason for the original design was and probably need to revert that change. As long as arbitrary transforms can insert intrinsics, we cannot require them to be part of the call graph.

I'm not sold on this, though it does not matter, see below. (W/o D141190 <https://reviews.llvm.org/D141190>, no intrinsics are ever represented. So, how does not representing them at all help us against the problem that occur if we do not representing them if they are added later?)

----

It looks like we do not need this after all. We might drop the assumptions (or the calls containing them) but that is fine. We cannot hoist them, and consequently the UB we assume not to happen will not be accidentally triggered. Other calls (intrinsic or not) that are inserted late, e.g., by the vectorizer, should similarly have less effects than the original code they replace, which should again allow the reasoning of @nikic to hold.

I'll close the bug as invalid.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155528/new/

https://reviews.llvm.org/D155528