[PATCH] D127209: [SVE][AArch64] Refine hasSVEArgsOrReturn

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.h:199
+    return HasSVEArgs ||
+           isa<ScalableVectorType>(MF.getFunction().getReturnType());
+  };
----------------
MattDevereau wrote:
> sdesmalen wrote:
> > efriedma wrote:
> > > sdesmalen wrote:
> > > > efriedma wrote:
> > > > > sdesmalen wrote:
> > > > > > efriedma wrote:
> > > > > > > MattDevereau wrote:
> > > > > > > > efriedma wrote:
> > > > > > > > > Hmm... actually, looking at this again, I'm a little concerned about checking the IR return type directly, instead of using AnalyzeReturn.  I'm not sure if this actually comes up in practice, but calling convention lowering does various transforms on the return type.  Maybe if you write something silly like "<vscale x 1000 x double>".
> > > > > > > > > 
> > > > > > > > > Maybe we can add a bit of code to LowerFormalArguments to also check the return type?
> > > > > > > > Modifying test `@sve_caller_sve_callee()` in `sve-tailcall.ll` to use return type `<vscale x 1000 x double>` caused the output "LLVM ERROR: Invalid size request on a scalable vector.".
> > > > > > > > 
> > > > > > > > In similar fashion to the changes in LowerFormalArguments, I checked the types of the return values in `AArch64TargetLowering::LowerReturn` instead of checking the IR return type directly. This ends up passing on the rest of the tests except `sve_caller_sve_callee` and `sve_caller_sve_callee_fastcc`, where the return type is not eligible for tail optimization and the frame-pointer is incorrectly unpreserved. Is directly checking the IR return type in `AArch64TargetLowering::LowerReturn` after `AnalyzeReturn` still not sufficient?
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > I don't think we call AArch64TargetLowering::LowerReturn for all functions, in general, only functions that return.  We need to do the computation somewhere that's called for every function.
> > > > > > > 
> > > > > > > Besides my `<vscale x 1000 x double>` example, I don't have any specific cases where just checking `isa<ScalableVectorType>` doesn't work, but I'd like to avoid falling out of sync if we do ever add support for, for example, returning fixed-width vectors in SVE registers.
> > > > > > > 
> > > > > > > > Modifying test @sve_caller_sve_callee() in sve-tailcall.ll to use return type <vscale x 1000 x double> caused the output "LLVM ERROR: Invalid size request on a scalable vector.".
> > > > > > > 
> > > > > > > That sounds like a bug we should fix.  Although maybe not a high priority one.
> > > > > > > I don't think we call AArch64TargetLowering::LowerReturn for all functions, in general, only functions that return. We need to do the computation somewhere that's called for every function.
> > > > > > I'm not sure I understand this point.
> > > > > > 
> > > > > > For arguments this check is done in LowerFormalArguments, so that case is all covered.  For the return value, I'd expect that LowerReturn is sufficient, because if the function doesn't return (and thus doesn't call LowerReturn), it also can't return a value in an SVE register.
> > > > > A function that doesn't return can still throw an exception.
> > > > If a function never returns because the only 'return' is an exceptional return, then it doesn't have to preserve the entire contents of z8-z23, which I think is what this patch tries to ascertain. Perhaps I was just reading too much into your statement, which confused me.  I would have expected the function that analyses/lowers the return to be the right place to handle this, if it is called at the right time of course.
> > > If we throw an exception, the unwinder has to restore z8-z23, I think?  Or is there some carveout for unwinding that lets us get away without preserving those registers somehow?  Is that documented somewhere?
> > > 
> > > There's also the issue that tail calls don't go through LowerReturn.
> > > 
> > > In any case, given that we're calling isSVECC() inside isel, we should ensure that all the setIsSVECC() happen before we start calling isSVECC(); anything else is confusing at best...
> > > If we throw an exception, the unwinder has to restore z8-z23, I think? Or is there some carveout for unwinding that lets us get away without preserving those registers somehow? Is that documented somewhere?
> > I'm not entirely sure where this is officially documented (or whether this is just common practice of existing unwinders), but D84737 states that unwinders may only preserve the lower 64bits.
> > 
> > > There's also the issue that tail calls don't go through LowerReturn.
> > >
> > > In any case, given that we're calling isSVECC() inside isel, we should ensure that all the setIsSVECC() happen before we start calling isSVECC(); anything else is confusing at best...
> > Yes, I agree.
> Is there any particular case where isSVECC() is likely to be called before setIsSVECC()? I've not observed this so far while working on this
isEligibleForTailCallOptimization() calls isSVECC(); I expect that runs before LowerReturn().


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127209