[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);
+}
----------------
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) ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85614