[PATCH] D156793: [TailCallElim] Remove the readonly attribute of byval.

[Alexey Lapshin via Phabricator via llvm-commits]

avl added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp:683
+      // attribute is removed.
+      F.removeParamAttr(I, Attribute::ReadOnly);
       ArgumentPHIs[I]->addIncoming(F.getArg(I), BB);
----------------
DianQK wrote:
> avl wrote:
> > probably, instead of removing readonly attribute we could stop generating it? Otherwise it will be presented or not depending of whether tail recursion elimination is happened.
> > 
> > ```
> > byval(<ty>)
> > 
> >     This indicates that the pointer parameter should really be passed by value to the function. The attribute implies that a hidden copy of the pointee is made between the caller and the callee, so the callee is unable to modify the value in the caller.
> > 
> > ```
> > 
> > The hidden copy protects original value from modifications done by callee. Having readonly attribute for hidden copy, probably, redundant.
> Do you mean don't add `readonly` at PostOrderFunctionAttrsPass?
> I don't think so. First rustc/clang can also add the `readonly` attribute.
> I think `readonly` + `byval` makes sense. There are more internal invariant representations than only `byval`.
if readonly is important then we probably should not do tail recursion elimination for readonly + byval as we will write into the readonly data. if it is not important then it should be safe to not set this flag?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156793