Page MenuHomePhabricator

[AArch64] Fix issues with large arrays on stack
ClosedPublic

Authored by kiranchandramohan on Nov 20 2019, 7:57 AM.

Details

Summary

This patch fixes a few issues when large arrays are allocated on the stack. Currently, clang has inconsistent behaviour, for debug builds there is an assertion failure when the array size on stack is around 2GB but there is no assertion when the stack is around 8GB. For release builds there is no assertion, the compilation succeeds but generates incorrect code. The incorrect code generated is due to using int/unsigned int instead of their 64-bit counterparts. This patch,

  1. Removes the assertion in frame legality check.
  2. Converts int/unsigned int in some places to the 64-bit variants. This helps in generating correct code and removes the inconsistent behaviour.
  3. Adds a test which runs without optimisations.

Diff Detail

Event Timeline

fpetrogalli added inline comments.
llvm/test/CodeGen/AArch64/large-stack.ll
2

Why can't you use the same prefix?

25

Do you need attribute #0 and #1?

The general approach here seems fine, I guess. At least, we shouldn't crash or miscompile, and generating code to allocate the full requested stack is probably the easiest way to do that without worrying about weird edge cases. If we really cared about this, we might want to consider some heuristics to "split" the stack so spill slots and small variables are cheap to access, but I doubt that's actually important in practice.

Realistically, I can't imagine any good result from code allocating an 8GB array on the stack; no operating system is going to allocate enough stack to make this work, at least by default.

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
1391–1392

Indentation here needs to be fixed.

llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.h
43

Is there a potential problem here as well?

llvm/test/CodeGen/AArch64/large-stack.ll
31

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?

kiranchandramohan marked 5 inline comments as done.Nov 22 2019, 7:54 AM
kiranchandramohan added a subscriber: eli.friedman.

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.

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
1391–1392

Will fix, thanks.

llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.h
43

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?

llvm/test/CodeGen/AArch64/large-stack.ll
2

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?

25

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?

31

Yes, we are doing the same thing here. That sequence is 128 long here.

fpetrogalli added inline comments.Nov 22 2019, 8:09 AM
llvm/test/CodeGen/AArch64/large-stack.ll
2

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]+]]
; ...
25

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! :)

efriedma added inline comments.Nov 22 2019, 1:17 PM
llvm/lib/Target/AArch64/AArch64MachineFunctionInfo.h
43

I guess not in practice; functions can't have that many arguments.

Addressed review comments by @fpetrogalli and @eli.friedman,

  1. Fixed formatting in llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
  2. Updated test to work with minimal attributes and removed prefixes for checks
kiranchandramohan marked 2 inline comments as done.Nov 25 2019, 4:17 AM
kiranchandramohan added inline comments.
llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
1391–1392

Done.

llvm/test/CodeGen/AArch64/large-stack.ll
25

Have now updated the test to have only the necessary attributes and also removed prefixes.

Does this patch need additional reviews or changes?

Please give reviewers at least a few days before you start pinging a patch. (I'll try to get to this today.)

Apologies, will wait for the reviews.

I'm a little concerned we've missed some conversion somewhere, but I don't have any good suggestion for that; we currently don't use integer conversion warnings in LLVM, and that would be a giant project to change.

There's some potential here that the arithmetic could overflow, even in 64 bits, but I'm not sure how we can handle that cleanly; I think it's okay to put off solving that issue.

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
917

Indentation.

1034

This should also be uint64_t?

llvm/test/CodeGen/AArch64/large-stack.ll
30

Using FileCheck variables for values that will never change isn't helpful. For example, "lsl #[[SHIFT:[0-9]+]]" is actually always "lsl #12", because that's the only legal value.

Is there a reason some of these CHECKs aren't CHECK-NEXT?

kiranchandramohan marked 6 inline comments as done.Nov 29 2019, 6:19 AM

In general, I also worry that i might have missed some checks. I was hoping to get some pointers on how to run some tests so that we can minimise this. At the same time, I also feel that this change should not cause regressions.

llvm/lib/Target/AArch64/AArch64FrameLowering.cpp
1034

Initially missed out since this portion of the code seems to error out. But you are correct that it has to be int64_t to prevent overflows.

llvm/test/CodeGen/AArch64/large-stack.ll
30

I have removed the variables from here.

A few of these are CHECKS and not CHECK-NEXT.

  1. Skips over some cfi attributes

; CHECK: sub x[[INDEX:[0-9]+]], x[[FRAME]], #8

  1. Skips over some set up for calling the print function

; CHECK: bl printf

  1. The CHECK-COUNTs did not seem to have a way of combining with NEXT.

; CHECK-COUNT-128: sub sp, sp, #[[STACK1:[0-9]+]], lsl #12
; CHECK-COUNT-128: add sp, sp, #[[STACK1]], lsl #12

Please let me know if this is not OK.

efriedma accepted this revision.Dec 3 2019, 5:39 PM

LGTM

This revision is now accepted and ready to land.Dec 3 2019, 5:39 PM
kiranchandramohan marked 2 inline comments as done.Dec 5 2019, 4:40 AM

Thanks @eli.friedman.
@fpetrogalli are you OK with the changes I made to your suggestions? Might need some handholding to land this patch.

@fpetrogalli are you OK with the changes I made to your suggestions? Might need some handholding to land this patch.

I am happy, with a nit: I think that CHECK-COUNT-128 is ignored? Or is COUNT-128 a specific token for the CHECK prefix?

@fpetrogalli are you OK with the changes I made to your suggestions? Might need some handholding to land this patch.

I am happy, with a nit: I think that CHECK-COUNT-128 is ignored? Or is COUNT-128 a specific token for the CHECK prefix?

Oh, I didn't know about this: https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-count-directive

The patch LGTM. Thanks!

This revision was automatically updated to reflect the committed changes.