Page MenuHomePhabricator

Improve stack-clash implementation on x86
ClosedPublic

Authored by serge-sans-paille on Wed, May 6, 4:49 AM.

Details

Summary
  • test both 32 and 64 bit version
  • probe the tail in dynamic-alloca
  • generate more concise code

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptWed, May 6, 4:49 AM
jonpa added a comment.Wed, May 6, 5:02 AM

This LGTM, but I would prefer if someone else working on X86 also gave an approval.

Rework and improve the patch:

  • more documentation
  • more assert
  • slight update of the way static alloca are probed, leads to slightly smaller code
  • completely rework the way dynamic alloca are probed, which leads to smaller and cleaner assembly. As a side effect it also probes the tail.
craig.topper added inline comments.Fri, May 15, 9:12 AM
llvm/lib/Target/X86/X86FrameLowering.cpp
717–719

This can't be MOV64rr for 32-bit. Can we use TargetOpcode::COPY?

llvm/lib/Target/X86/X86ISelLowering.cpp
31650

use Register instead of unsigned.

31657–31664

This shouldn't be MOV64rr if the stack pointer is 32 bits. Can we just use TargetOpcode::COPY here?

Take remarks into account

serge-sans-paille marked 3 inline comments as done.Fri, May 15, 1:12 PM

Can we have tests for 32-bit mode too? Which I hope would have caught those previous issues.

llvm/lib/Target/X86/X86FrameLowering.cpp
664–665

Register

serge-sans-paille retitled this revision from Fix stack clash probing on the tail of dynamic allocation to Improve stack-clash implementation on x86.
serge-sans-paille edited the summary of this revision. (Show Details)
serge-sans-paille marked an inline comment as done.

@craig.topper test cases for 32bit target added!

This revision is now accepted and ready to land.Mon, May 25, 1:12 AM
lkail added a subscriber: lkail.Mon, May 25, 5:47 AM
lkail removed a subscriber: lkail.
This revision was automatically updated to reflect the committed changes.