[PATCH] D84220: [IPSCCP] Fix a bug that the "returned" attribute is not cleared when function is optimized to return undef

[Congzhe Cao via Phabricator via llvm-commits]

congzhe added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:2042
+    for (Argument& A : F->args())
+      F->removeParamAttr(A.getArgNo(), Attribute::Returned);
   }
----------------
jdoerfert wrote:
> fhahn wrote:
> > A single function can have multiple returns to 'zap' and we would repeatedly checked the same function unnecessarily. 
> > 
> > It might be better to remove the returned argument once for each function we found returns to zap. This could either be done by checking if findReturnsToZap found returns to zap or by collecting a set of functions for which zapped returns and then remove the Returned attribute for their arguments.
> call sites can have the attribute too.
Thanks, added code that removes returned attribute for call sites


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:2042
+    for (Argument& A : F->args())
+      F->removeParamAttr(A.getArgNo(), Attribute::Returned);
   }
----------------
congzhe wrote:
> jdoerfert wrote:
> > fhahn wrote:
> > > A single function can have multiple returns to 'zap' and we would repeatedly checked the same function unnecessarily. 
> > > 
> > > It might be better to remove the returned argument once for each function we found returns to zap. This could either be done by checking if findReturnsToZap found returns to zap or by collecting a set of functions for which zapped returns and then remove the Returned attribute for their arguments.
> > call sites can have the attribute too.
> Thanks, added code that removes returned attribute for call sites
Thanks, now keep a set of functions whose returns are zapped, and work on functions in the set afterwards


================
Comment at: llvm/test/Transforms/SCCP/ipsccp-clear-returned.ll:11
+  %call = call i32 @func_return_undef(i32 1)
+; CHECK: {{call.*@func_return_undef}}
+  ret i32 0
----------------
fhahn wrote:
> Using a regular expression here looks a bit odd. Maybe `; CHECK: call {{.*}} @func_return_undef` or maybe even better spell out the full call? Also it might be good to return the result value `ret i32 %call` and check if it gets replaced with `1`.
Thank you, updated accordingly.


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

https://reviews.llvm.org/D84220