[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);
----------------
DianQK wrote:
> 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.
> 
> 
> But that doesn't really impact this patch: unless we actually make "readonly byval" illegal, TCE needs to handle it anyway.

So I think I'd go ahead and submit this patch to fix the miscompilation. If anyone has other ideas they can submit a new patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156793