[PATCH] D37460: [GVN] Prevent LoadPRE from hoisting across instructions that don't pass control flow to successors

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev marked an inline comment as done.
mkazantsev added inline comments.


================
Comment at: test/Transforms/GVN/PRE/local-pre.ll:48
+  call void @may_exit() nounwind
+  %b = sdiv i32 %p, %q
+  ret i32 %b
----------------
efriedma wrote:
> mkazantsev wrote:
> > efriedma wrote:
> > > mkazantsev wrote:
> > > > mkazantsev wrote:
> > > > > efriedma wrote:
> > > > > > GVN got smarter, so this isn't actually testing what it's supposed to test anymore.  Maybe try replacing "icmp eq i32 %p, %q" with "icmp eq i32 %p, %r", where "%r" is a parameter.  And I guess pass %a to may_exit(), so it won't get moved if we come up with some new form of sinking in the future.
> > > > > What is the difference between `icmp eq i32 %p, %q` where `%q` is a parameter and `icmp eq i32 %p, %r` where `%r` is a parameter? If you mean that we should branch by unknown condition then we could just add parameter `i5 %cond`.
> > > > > 
> > > > > Anyways, I don't think that extension of the test set can somehow block us from merging this functional fix. I also don't see what is the conceptual difference between this test and what you describe. The transformation across `may_exit` still doesn't happen. Do you mind if we consider adding new tests as NFC later?
> > > > `%i1` cond off course :)
> > > `i1 %cond` probably does the same thing, yes.  The important part is that we don't simplify `sdiv i32 %p, %q` to 1.
> > > 
> > > Fixing the underlying issue here doesn't necessarily block committing this patch, I guess... but it's probably something you want to address at some point, and it's very closely related to this patch.
> > I want to clearly distinguish fixing miscompile with guards and a missing opportunity for turning this `div` into `1`. Do we have a bug to address this missing opportunity? You could file it if you think this case is important. But I don't think it is a part of the problem being addressed by this patch.
> Sorry, probably should have written this out in the first place.
> 
> GVN currently performs an incorrect optimization for the following testcase:
> 
> ```
> define i32 @test2(i32 %p, i32 %q, i1 %r) {
> block1:
>   br i1 %r, label %block2, label %block3
> 
> block2:
>  %a = sdiv i32 %p, %q
>  br label %block4
> 
> block3:
>   br label %block4
> 
> block4:
>   %phi = phi i32 [ 0, %block3 ], [ %a, %block2 ]
>   call void @may_exit(i32 %phi) nounwind
>   %b = sdiv i32 %p, %q
>   ret i32 %b
> 
> }
> declare void @may_exit(i32) nounwind
> ```
Ah, now I got what you mean. :) Thanks for pointing out! Fixed.



https://reviews.llvm.org/D37460