This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Implement dynamic stack probing for windows
ClosedPublic

Authored by mstorsjo on Jan 21 2018, 12:53 PM.

Details

Summary

This makes sure that alloca() function calls properly probe the stack as needed.

This is partially based on D40863 by @aemerson.

Diff Detail

Event Timeline

mstorsjo created this revision.Jan 21 2018, 12:53 PM

For GlobalISel we haven't had to have very target specific code in the IRTranslator for this before. @qcolombet any opinion on whether we want to include target/ABI specific code into the IRTranslator? My feeling is that we want to avoid this, the alternative I can see is to introduce a new opcode for G_DYNALLOCA and lower that at isel.

lib/Target/AArch64/AArch64ISelLowering.cpp
7452

Unfortunately it seems we need to pass a const char* to this because of lifetime issues at codegen. Do you have any ideas on how to do this better?

7458

Can you separate this out into some kind of helper for the Windows specific ABI?

For GlobalISel we haven't had to have very target specific code in the IRTranslator for this before. @qcolombet any opinion on whether we want to include target/ABI specific code into the IRTranslator? My feeling is that we want to avoid this, the alternative I can see is to introduce a new opcode for G_DYNALLOCA and lower that at isel.

Using a new opcode that maps down to this case here probably sounds like the best option.

lib/Target/AArch64/AArch64ISelLowering.cpp
7452

I don't think I'm following what the issue is here? Right now I'm passing "__chkstk" as a const char* here. Do you mean further, if refactoring this into a form that also suits darwin, with a different function name? In that case, the method providing the function name could probably just return const char* as well, instead of e.g. a StringRef.

7458

I can try to make it easier fit in the darwin version from D40863, by more clearly separating out the windows specific bits.

test/CodeGen/AArch64/win64_vararg.ll
171

This form produces the pattern of "mov x8, sp; sub x20, x8, x15, lsl #4; mov sp x20", when it could just as well do "sub sp, sp, x15, lsl #4; mov x20, sp" - however I didn't manage to make the code in AArch64ISelLowering do that - any hints?

The version I did in D41131 for the fixed stack parts use BuildMI(MBB, MBBI, DL, TII->get(AArch64::SUBXrx64), AArch64::SP).addReg(AArch64::SP, RegState::Kill).addReg(AArch64::X15, RegState::Kill).addImm(AArch64_AM::getArithExtendImm(AArch64_AM::UXTX, 4)) - which achieves exactly what I want.

aemerson added inline comments.Jan 25 2018, 6:15 AM
lib/Target/AArch64/AArch64ISelLowering.cpp
7452

Sorry I didn't word that right. I meant that when I tried to use the getStackProbingSymbol hook from TargetLowering it returns a StringRef, so I had to hard code the string in this function as a short term solution. It's ok if we only need it for one platform, but it would be nice to have it configurable by the specific platform.

test/CodeGen/AArch64/win64_vararg.ll
171

I don't know of any way to do that without using getCopyFromReg, not to say there definitely isn't a better way.

mstorsjo updated this revision to Diff 133838.Feb 12 2018, 5:23 AM
mstorsjo edited the summary of this revision. (Show Details)

Structured the code to easier allow adding a darwin specific version of stack probing without conflicting with the windows specific parts here. Added a "return false" in GlobalISel for windows for alloca, to make sure we invoke the proper codegen for this.

On ARM and X86, the implementation of stack probing uses a custom opcode like WIN__CHKSTK - that would allow to more directly emit exactly the intended machine code for this call. Doing it via SelectionDAG is less code but ends up less elegant at -O0 - a compromise I think is ok.

aemerson accepted this revision.Feb 16 2018, 10:50 PM

Thanks for refactoring it. LGTM with a minor comment addition.

lib/CodeGen/GlobalISel/IRTranslator.cpp
1007

Please add a comment here explaining why Windows isn't supported.

This revision is now accepted and ready to land.Feb 16 2018, 10:50 PM
This revision was automatically updated to reflect the committed changes.