[PATCH] D70496: [AArch64] Fix issues with large arrays on stack

[Kiran Chandramohan via Phabricator via llvm-commits]

kiranchandramohan marked 5 inline comments as done.
kiranchandramohan added a subscriber: eli.friedman.
kiranchandramohan added a comment.

Thanks @fpetrogalli, @eli.friedman for the review.

I have responded to the review comments. I have a few questions and will update with a new patch after your response to these questions.



================
Comment at: llvm/lib/Target/AArch64/AArch64FrameLowering.cpp:1395
+  int64_t NumBytes = IsFunclet ? getWinEHFuncletFrameSize(MF)
                            : MFI.getStackSize();
   AArch64FunctionInfo *AFI = MF.getInfo<AArch64FunctionInfo>();
----------------
efriedma wrote:
> Indentation here needs to be fixed.
Will fix, thanks.


================
Comment at: llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.h:43
   /// callee.
   unsigned BytesInStackArgArea = 0;
 
----------------
efriedma wrote:
> Is there a potential problem here as well?
I don't know whether i can answer that. Typically the ABI prohibits passing large objects by value. For e.g. the AArch64 ABI disallows passing composite types by value whose size is larger than 16bytes.
"If the argument type is a Composite Type that is larger than 16 bytes, then the argument is copied to memory allocated by the caller and the argument is replaced by a pointer to the copy."

If this refers to arguments only then probably it is not needed. 

Do you feel there are cases which can cause this variable to have a high value which requires 64 bit?


================
Comment at: llvm/test/CodeGen/AArch64/large-stack.ll:1
+; RUN: llc < %s -mtriple=aarch64-none-linux-gnu | FileCheck %s --check-prefixes=CHECK-SPILL,CHECK-RELOAD,CHECK-FRAME,CHECK-SETSTACK,CHECK-RESETSTACK,CHECK-VAL,CHECK-CALL
+
----------------
fpetrogalli wrote:
> Why can't you use the same prefix?
So that i can progressively construct the test. First check the spills, then the stack and the then accessing the value. Are you recommending combining them?


================
Comment at: llvm/test/CodeGen/AArch64/large-stack.ll:24
+
+attributes #0 = { noinline nounwind optnone uwtable "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "min-legal-vector-width"="0" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-jump-tables"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="generic" "target-features"="+neon" "unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { "correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" "less-precise-fpmad"="false" "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" "no-infs-fp-math"="false" "no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" "no-trapping-math"="false" "stack-protector-buffer-size"="8" "target-cpu"="generic" "target-features"="+neon" "unsafe-fp-math"="false" "use-soft-float"="false" }
----------------
fpetrogalli wrote:
> Do you need attribute #0 and #1?
Maybe not. But these attributes are by default generated by clang and hence matches the IR that clang generates. 

Also, the test will need updating since the attribute specified uses some attributes (no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf") which can change the assembly code generated.

What is the standard for these tests do you omit attributes?


================
Comment at: llvm/test/CodeGen/AArch64/large-stack.ll:30
+; CHECK-FRAME:                mov	x[[FRAME:[0-9]+]], sp
+; CHECK-SETSTACK-COUNT-128:   sub	sp, sp, #[[STACK1:[0-9]+]], lsl #[[SHIFT:[0-9]+]]
+; CHECK-SETSTACK-NEXT:        sub	sp, sp, #[[STACK2:[0-9]+]], lsl #[[SHIFT]]
----------------
efriedma wrote:
> What sequence are we generating to adjust the stack pointer?  For a large array that fits in 32 bits, I see a long sequence of  "sub sp, sp, #4095, lsl #12"; are we doing the same thing here?
Yes, we are doing the same thing here. That sequence is 128 long here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70496