Page MenuHomePhabricator

[VE] Dynamic stack allocation
ClosedPublic

Authored by simoll on Apr 29 2020, 6:10 AM.

Details

Summary

This patch implements dynamic stack allocation for the VE target. Changes:

  • compiler-rt: __ve_grow_stack to request stack allocation on the VE.
  • VE: base pointer support, dynamic stack allocation.

Diff Detail

Event Timeline

simoll created this revision.Apr 29 2020, 6:10 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 29 2020, 6:10 AM
Herald added subscribers: llvm-commits, Restricted Project, s.egerton and 6 others. · View Herald Transcript
arsenm added inline comments.Apr 29 2020, 8:15 AM
llvm/lib/Target/VE/VEInstrInfo.cpp
555

getSubtarget<VESubtarget> and remove the cast. You also added Subtarget as a member, so you don't need the get anyway

563

getSubtarget<VESubtarget>() and use the VEFrameLowering?

simoll updated this revision to Diff 260970.Apr 29 2020, 11:27 AM
simoll marked 2 inline comments as done.
  • @arsenm 's comments
  • rebased
  • fixed commit attribution (to @kaz7 )
simoll updated this revision to Diff 261752.May 4 2020, 12:55 AM
  • rebased
  • formatting
simoll added a comment.May 5 2020, 5:13 AM

ping. Any more comments for this?

arsenm added inline comments.May 5 2020, 7:22 AM
llvm/include/llvm/IR/CallingConv.h
244–247 ↗(On Diff #261752)

Seems weird to me that this needs its own CC, or that it would have llvm in the name. This should be split into a separate patch, and needs some bitcode encoding tests

simoll planned changes to this revision.May 5 2020, 10:40 AM

Will try to get rid of the custom CC. Thanks for having another look at this!

simoll updated this revision to Diff 262581.EditedMay 7 2020, 3:02 AM
simoll edited the summary of this revision. (Show Details)
  • use standard C CC for grow_stack builtins.
  • added test for aligned stack allocation.
  • renamed builtins to __builtin_grow_stack[_align].
  • rebased.
arsenm added inline comments.May 11 2020, 5:16 PM
compiler-rt/lib/builtins/ve/grow_stack_align.S
18

Calling this builtin seems a bit wrong to me. builtin is usually a clang/C/frontend wrapper around an IR intrinsic. __ve_grow_stack_align? If it's already called this in some ABI spec then it doesn't matter

simoll updated this revision to Diff 263386.May 12 2020, 2:47 AM
simoll edited the summary of this revision. (Show Details)
  • Renamed to __ve_grow_stack . The sequence has no specific name in the documentation, we can simply use a more palatable name.
  • Minor cleanup.
  • Rebased.
simoll updated this revision to Diff 263635.May 13 2020, 12:23 AM
  • made lint a little happier with camelCaseFuncNames.
  • rebased.
kaz7 added a comment.May 18 2020, 5:27 PM
  • use standard C CC for grow_stack builtins.

@simoll @arsenm

Sorry for late reply. I originally want to use it's own calling convention to allow caller to not spill registers since many program may call this function to grow their stack. Is it not acceptable in general? Or it's OK if there is a reason?

It's OK to upstreaming C CC as a first step. I would like to know what we should do to improve the performance of alloca or dynamic stack allocation in C++. Thanks!

  • use standard C CC for grow_stack builtins.

@simoll @arsenm

Sorry for late reply. I originally want to use it's own calling convention to allow caller to not spill registers since many program may call this function to grow their stack. Is it not acceptable in general? Or it's OK if there is a reason?

It's OK to upstreaming C CC as a first step. I would like to know what we should do to improve the performance of alloca or dynamic stack allocation in C++. Thanks!

Would this just be the same as the existing PreserveMost or PreserveAll conventions?

kaz7 added a comment.May 18 2020, 9:25 PM

@arsenm Yes! I didn't notice those calling conventions. We can use them as our target depend special calling conventions. Thank you for informing us.

simoll updated this revision to Diff 264850.May 19 2020, 4:28 AM
  • Use 'PreserveAll' CC for ve_grow_stack.
  • Rebased.
kaz7 added a comment.May 19 2020, 10:32 PM

verifyLeafProcRegUse function is not used anymore.

llvm/lib/Target/VE/VEFrameLowering.cpp
351

Remove this function from upstreaming. This is obsoleted function.

simoll updated this revision to Diff 265162.May 20 2020, 1:16 AM
simoll marked an inline comment as done.

remove verifyLeafProcRegUse

simoll updated this revision to Diff 266032.May 25 2020, 8:37 AM
  • NFC, Lint issues.
  • rebased.

Any feedback for this one?

arsenm accepted this revision.May 26 2020, 8:00 AM

LGTM with nits

llvm/lib/Target/VE/VEFrameLowering.cpp
336

Else after return

344

No else after return

This revision is now accepted and ready to land.May 26 2020, 8:00 AM
simoll updated this revision to Diff 266443.May 27 2020, 12:38 AM
simoll marked 3 inline comments as done.
  • Fixed else after return
  • Rebased

Thanks!

This revision was automatically updated to reflect the committed changes.