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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert marked 3 inline comments as done.
jdoerfert added a comment.

Some context: 
I did not write these tests but copied them and changed the run line. Given that was not the plan initially when we started the review I didn't want to just go ahead.
The plan now is to copy them, this patch, then run update_check on them, then use them to improve coverage once IPCP features trickle into the Attributor.



================
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]
----------------
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.


================
Comment at: llvm/test/Transforms/Attributor/IPConstantProp/musttail-call.ll:48
+  ; The call to this function is removed, so the return value must be zapped
+  ; CHECK: ret i8* null
+  ret i8* null
----------------
efriedma wrote:
> Comment isn't consistent with CHECK?
I'll get rid of the comment.


================
Comment at: llvm/test/Transforms/Attributor/IPConstantProp/user-with-multiple-uses.ll:5
+; IPSCCP should propagate the 0 argument, eliminate the switch, and propagate
+; the result.
+
----------------
efriedma wrote:
> This is the same test as remove-call-inst.ll?
It seem that way, I'll remove it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69748