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

[DianQK via Phabricator via llvm-commits]

DianQK 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);
----------------
efriedma wrote:
> 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.
> 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?

It is possible to perform a transformation in a way that prevents another, larger optimization.
readonly must make sense. At least until tail recursion elimination, other passes can use readonly.
But I think tail recursion elimination has a better opportunity for optimization than readonly.
In the test case, with tail recursion elimination eventually converted to a ret instruction.

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

Using readonly we can avoid reusing MemorySSA analysis. This should reduce compilation time. And that covers all cases.
readonly + byval is indeed confusing. But I think this discussion we can clarify the meaning.
- readonly = Indicates invariance to *internal* and external.
- byval = Indicates invariance to external.


================
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:
> efriedma wrote:
> > 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.
> > 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?
> 
> It is possible to perform a transformation in a way that prevents another, larger optimization.
> readonly must make sense. At least until tail recursion elimination, other passes can use readonly.
> But I think tail recursion elimination has a better opportunity for optimization than readonly.
> In the test case, with tail recursion elimination eventually converted to a ret instruction.
> 
> > 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.
> 
> Using readonly we can avoid reusing MemorySSA analysis. This should reduce compilation time. And that covers all cases.
> readonly + byval is indeed confusing. But I think this discussion we can clarify the meaning.
> - readonly = Indicates invariance to *internal* and external.
> - byval = Indicates invariance to external.
> 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