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

[Eli Friedman via Phabricator via llvm-commits]

efriedma 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);
----------------
avl wrote:
> 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?
Marking a byval pointer readonly isn't very useful; MemorySSA can easily analyze a byval argument without the marking in almost all cases.

I would tend towards saying we shouldn't add readonly markings to byval values, if only to reduce the chance of this sort of confusion.  But that doesn't really impact this patch: unless we actually make "readonly byval" illegal, TCE needs to handle it anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156793