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

Eli Friedman via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Dec 13 11:28:25 PST 2019


efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM



================
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:
> 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)
Okay.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69748





More information about the llvm-commits mailing list