[PATCH] D34387: [PATCH 2/2] Implement "probe-stack" on x86

[whitequark via Phabricator via llvm-commits]

whitequark added inline comments.


================
Comment at: lib/Target/X86/X86FrameLowering.cpp:1253
+    if (SpillR11) {
+      pushRegForStackProbeCall(MF, MBB, MBBI, DL, R11Alive, X86::R11, NumBytes);
     }
----------------
pcwalton wrote:
> majnemer wrote:
> > Hmm, I believe a testcase for this situation hasn't been added.
> Well, I'm not sure how R11 can actually be live-in in the function prolog right now. As I understand things, the reason why EAX is spilled if necessary is to handle the edge case on 32-bit x86 where `inreg` is specified for a function parameter, which causes EAX to be live-in at the very start of the function. But there's no corresponding `inreg` attribute for x86-64 that causes R11 to become live-in that early. Checking for that case even for R11 seemed prudent to me though, because (a) the corresponding situation can happen for EAX and it seemed cleaner to use the same logic for both registers instead of duplicating some of it; (b) in the future, if somehow R11 can become live-in at function start, this logic will be needed.
> 
> Am I interpreting the situation correctly, and if so, what would you prefer I do regarding testing?
I am quite certain that R11 should *not* be spilled this way. LLVM uses it as a scratch register in quite a few places. E.g. the comment above:

    // For the large code model, we have to call through a register. Use R11,
    // as it is scratch in all supported calling conventions.

the GetScratchRegister function in X86FrameLowering.cpp, the X86TargetLowering::getScratchRegisters, and the X86TargetLowering::LowerINIT_TRAMPOLINE function, which needs it for essentially the same job.

You could perhaps replace the call with one of the getScratchRegister functions but it seems fine to hardcode it while not spilling the way it is, too.


https://reviews.llvm.org/D34387