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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/TailRecursionElimination.cpp:610
+                       CI->getArgOperand(OpndIdx),
+                       /*SrcAlign*/ Alignment, Size);
+}
----------------
avl wrote:
> 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?
I don't think you need to mix in getPrefTypeAlignment.  I mean, it's not wrong to use a lower alignment than StackSlotAlignment, but there isn't any reason you'd want to.

Instead of `max(1, MaybeAlign(CI->getParamAlign(OpndIdx))`, you'd write `CI->getParamAlign(OpndIdx).valueOrOne()`, but sure, that's correct.  The code quality might be a bit iffy for byval without alignment, but nothing should be generating code like that anyway, so I guess it's fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85614