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

[Francesco Petrogalli via Phabricator via llvm-commits]

fpetrogalli added inline comments.


================
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
+
----------------
kiranchandramohan wrote:
> 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?
I can see value in having multiple prefixes when building the test. But multiple prefixes are designed to be used when executing multiple RUN lines on the same source. If you remove them the tests will be more clear.

If you need to highlight what you are checking, I'd rather add plain comments inline where the CHECKs are:

```
; Checking spill
; CHECK:                stp  x[[SPILL_REG1:[0-9]+]], x[[SPILL_REG2:[0-9]+]], [sp, #-[[SPILL_OFFSET1:[0-9]+]]]
; CHECK-NEXT:           str  x[[SPILL_REG3:[0-9]+]], [sp, #[[SPILL_OFFSET2:[0-9]+]]]
; Checking frame
; CHECK:                mov  x[[FRAME:[0-9]+]], sp
; Checking setstack count 128 bit
; CHECK:   sub  sp, sp, #[[STACK1:[0-9]+]], lsl #[[SHIFT:[0-9]+]]
; ...
```


================
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" }
----------------
kiranchandramohan wrote:
> 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?
If you need attributes, you should reduce them to the minimal set needed for the test. If `no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf"` is all you need, remove the rest (and merge the two attributes in a single attribute). less is always better! :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70496