[PATCH] D85614: [TRE] Reland: allow TRE for non-capturing calls.

[Alexey Lapshin via Phabricator via llvm-commits]

avl added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp:610
+                       CI->getArgOperand(OpndIdx),
+                       /*SrcAlign*/ Alignment, Size);
+}
----------------
avl wrote:
> efriedma wrote:
> > I think you need to be more careful with alignment here: it's UB if you specify alignment higher than the actual alignment of the value.  For byval arguments with the alignment set, the alignment is exactly that.
> > 
> > For byval arguments where the alignment attribute is missing, probably fine to just refuse to do TRE.  It's an edge case which shouldn't really come up in practice.
> > I think you need to be more careful with alignment here: it's UB if you specify alignment higher than the actual alignment of the value.  For byval arguments with the alignment set, the alignment is exactly that.
> > 
> so, instead of this:
> 
>   Align Alignment(DL.getPrefTypeAlignment(AggTy));
>   Alignment = max(Alignment, MaybeAlign(CI->getParamAlign(OpndIdx)));
> 
> I need to do just this :
> 
>   Align Alignment(DL.getPrefTypeAlignment(AggTy));
> 
> right?
> 
> > For byval arguments where the alignment attribute is missing, probably fine to just refuse to do TRE.  It's an edge case which shouldn't really come up in practice.
> 
> If alignment attribute is missing would it be OK to use Align(1) ?
i.e. both above conditions would look like this:


```
Align StackSlotAlignment(max(1, MaybeAlign(CI->getParamAlign(OpndIdx))));
Align FinalAlignment(min(DL.getPrefTypeAlignment(AggTy), StackSlotAlignment));

```

would it be correct?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85614