[PATCH] D69748: [Attributor][IPConstantProp] Run the Attributor on IPConstantProp tests

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/test/Transforms/Attributor/IPConstantProp/2009-09-24-byval-ptr.ll:24
+  %1 = load i32, i32* %0
+; CHECK: %struct.MYstr* @mystr, i{{..}} 0, i32 1
+  %2 = getelementptr %struct.MYstr, %struct.MYstr* %u, i32 0, i32 0 ; <i8*> [#uses=1]
----------------
jdoerfert wrote:
> efriedma wrote:
> > jdoerfert wrote:
> > > efriedma wrote:
> > > > This is technically a legal optimization given the test as written, but I don't think you're proving it correctly.  readonly on a function does not imply readonly on its byval arguments.  Try the following variation:
> > > > 
> > > > ```
> > > > define internal i32 @vfu2(%struct.MYstr* byval align 4 %u) nounwind readonly {
> > > > entry:
> > > >   %z = getelementptr %struct.MYstr, %struct.MYstr* %u, i32 0, i32 1
> > > >   store i32 99, i32* %z, align 4
> > > >   %0 = getelementptr %struct.MYstr, %struct.MYstr* %u, i32 0, i32 1 ; <i32*> [#uses=1]
> > > >   %1 = load i32, i32* %0
> > > >   %2 = getelementptr %struct.MYstr, %struct.MYstr* %u, i32 0, i32 0 ; <i8*> [#uses=1]
> > > >   %3 = load i8, i8* %2
> > > >   %4 = zext i8 %3 to i32
> > > >   %5 = add i32 %4, %1
> > > >   ret i32 %5
> > > > }
> > > > ```
> > > The Attributor doesn't really do anything on this test yet. Only after pointer privatization goes in. I'll add your test case though.
> > I probably should have posted a complete testcase.  I meant taking this testcase, and replacing the vfu2 definition with my function.  It looks like attributor transforms it.  (Unless I messed up somehow.)
> Here is the test, original and your version (_2), https://godbolt.org/z/A6L--T
> 
> What transformation do you mean and do you think it is wrong?
The transform of the GEP from `%z = getelementptr %struct.MYstr, %struct.MYstr* %u, i32 0, i32 1` to `%z = getelementptr %struct.MYstr, %struct.MYstr* @mystr, i32 0, i32 1`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69748