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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert marked an inline comment as done.
jdoerfert 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:
> > > > 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`.
> Sorry, I must have been blind.
> 
> The readonly on the function should not imply readonly on the byval argument because it is copied. Agreed. I'll fix that.
This should be fixed with  rG6abd01e4624a2c9f8f76e11cc5d57cc7551b5d2a. I'll rerun the check scripts before I commit and verify the diff (if that is OK with you)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69748