Support for HiPE-compatible code emission
Yiannis Tsiouris
gtsiour at softlab.ntua.gr
Wed Feb 13 09:11:13 PST 2013
Hi Eli,
Thanks for taking some time to review this! :-) Look for my answers inline:
On 02/11/2013 09:25 PM, Eli Bendersky wrote:
> diff --git a/include/llvm/Target/TargetFrameLowering.h
> b/include/llvm/Target/TargetFrameLowering.h
> index 1958f90..32d72e7 100644
> --- a/include/llvm/Target/TargetFrameLowering.h
> +++ b/include/llvm/Target/TargetFrameLowering.h
> @@ -120,6 +120,11 @@ public:
> /// by adding a check even before the "normal" function prologue.
> virtual void adjustForSegmentedStacks(MachineFunction&MF) const { }
>
> + /// Adjust the prologue to add HiPE specific code that explicitly handles the
> + /// stack. This works by adding a check and a call to "inc_stack_0"
> Erlang BIF
> + /// before the normal function prologue.
> + virtual void adjustForHiPEPrologue(MachineFunction&MF) const { }
> +
>
> I don't think the implementation details belong in the comment for the generic
> target-independent method. Leave them for the actual implementation.
>
ACK.
> Also, I'm concerned about this growing "adjustFor<XXX>" trend. I won't heap this
> on your patch, but any ideas about generalizing this are appreciated. Perhaps
> some way to register options -> adjust methods.
>
I was thinking about that and IMO I'm not familiar-enough with the LLVM
code base to suggest a more general approach. Thus, I'd prefer to keep
it like this for now, if you don't mind, and come back to this once
someone has a better idea. :)
> diff --git a/lib/Target/X86/X86FrameLowering.cpp
> b/lib/Target/X86/X86FrameLowering.cpp
> index 420aeb8..f1d244a 100644
> --- a/lib/Target/X86/X86FrameLowering.cpp
> +++ b/lib/Target/X86/X86FrameLowering.cpp
> @@ -1387,11 +1387,16 @@ HasNestArgument(const MachineFunction *MF) {
> /// either one or two registers will be needed. Set primary to true for
> /// the first register, false for the second.
> static unsigned
> -GetScratchRegister(bool Is64Bit, const MachineFunction&MF, bool Primary) {
> +GetScratchRegister(bool Is64Bit, const MachineFunction&MF, bool
> Primary=true) {
>
> The comment above the function mentions that it's needed fro segmented stacks.
> You now use it for a different purpose. Please modify the commento to reflect
> that.
>
ACK.
> + CallingConv::ID CallingConvention = MF.getFunction()->getCallingConv();
> +
> + // R14/EBX is a temp register, not used in HiPE Calling Convention.
> + if (CallingConvention == CallingConv::HiPE)
> + return (Is64Bit ? X86::R14 : X86::EBX);
> +
>
> Can this be added into the existing if (CallingConvention) condition chain?
>
Done. (+ add 2nd temp register)
> +// Erlang programs may need a special prologue to handle the stack size they
> +// might need at runtime. That is because Erlang/OTP does not implement a C
> +// stack but uses a custom implementation of hybrid stack/heap
> +// architecture. (for more information see Eric Stenman's Ph.D. thesis:
> +// http://publications.uu.se/uu/fulltext/nbn_se_uu_diva-2688.pdf)
> +//
> +//
> +// CheckStack:
> +// temp0 = sp - MaxStack
> +// if( temp0< SP_LIMIT(P) ) goto IncStack else goto OldStart
> +// OldStart:
> +// ...
> +// IncStack:
> +// call inc_stack # doubles the stack space
> +// temp0 = sp - MaxStack
> +// if( temp0< SP_LIMIT(P) ) goto IncStack else goto OldStart
> +void X86FrameLowering::adjustForHiPEPrologue(MachineFunction&MF) const {
> + const X86InstrInfo&TII = *TM.getInstrInfo();
> + const X86Subtarget *ST =&MF.getTarget().getSubtarget<X86Subtarget>();
> + MachineFrameInfo *MFI = MF.getFrameInfo();
> + const uint64_t SlotSize = TM.getDataLayout()->getPointerSize(0);
> + const bool Is64Bit = STI.is64Bit();
>
> Tests indicate that you care about the x32 ABI, where sometimes you want to
> know the data model and not the platform bitness. This applies to stack slot
> size and to registers used for addresses. How does this affect your code?
>
I'm not sure I get your question here. Let me clarify what I actually
care about as this misunderstanding might be a result of my abuse of the
LLVM API:
> + const unsigned Guaranteed = HipeLeafWords * SlotSize;
> + const unsigned CallerStkArity =
> + std::max<int>(0, MF.getFunction()->arg_size() - CCRegisteredArgs);
> + unsigned MaxStack =
> + MFI->getStackSize() + CallerStkArity * SlotSize + SlotSize;
>
I 'd like to compute some metrics (such as the above) based on the
number of *bytes* that are occupied on the stack for some values (e.g.,
the number of bytes that are used on the stack for the arguments of a
function). One way to do that (that seemed reasonable to me :-)) was to
use the pointer-size (or word-size or slot-size). So, what I only care
about is the platform bitness. Is there a better way to write that? :-)
> + if (!ST->isTargetLinux())
> + report_fatal_error("HiPE prologue not supported on this platform.");
>
> Don't you have an earlier place for this runtime test? I would assume it should
> not get this far if HiPe codegen is attempted for non-Linux platforms. At this
> point I'd expect an assertion.
>
Your assumption is right; changed this to an assertion.
> +
> + // Make sure 'Guaranteed'-area of callee fits in caller's frame
> + if (MFI->hasCalls()) {
> + unsigned MoreStackForCalls = 0;
> + for(MachineFunction::iterator MBBI = MF.begin(), MBBE = MF.end();
>
> for (MachineFunction
>
> + MBBI != MBBE; ++MBBI)
> + for (MachineBasicBlock::iterator MI = MBBI->begin(), ME = MBBI->end();
> + MI != ME; ++MI)
> + if(MI->isCall()) {
>
> if (MI->isCall)
> Similarly, fix this in other instances in the patch.
>
> + // Get callee operand
>
> Indentation?
>
All fixed.
> + const MachineOperand&MO = MI->getOperand(0);
> + const Function *F;
> +
> + // Only take account of global function calls (no closures etc)
> + if (!MO.isGlobal()) continue;
> + if (!(F = dyn_cast<Function>(MO.getGlobal()))) continue;
> +
> + // Do not update MaxStack for primitive and built-in functions
> + if ((F->getName().find("erlang.") != std::string::npos) ||
> + (F->getName().find("bif_") != std::string::npos)) continue;
> + if (F->getName().find_first_of("._") == std::string::npos)
> + continue;
> +
> + const uint64_t CalleeStkArity =
> + std::max<int64_t>(0, F->arg_size()-CCRegisteredArgs);
> + MoreStackForCalls = std::max<int64_t>(
> + MoreStackForCalls,
> (HipeLeafWords-1-CalleeStkArity)*SlotSize);
> + }
> + MaxStack += MoreStackForCalls;
> + }
> +
>
> Some high-level comment explaining what's going on would be nice here.
>
I added a short description. I hope this makes more sense!
> + if (MaxStack> Guaranteed) {
> + MachineBasicBlock&prologueMBB = MF.front();
> + MachineBasicBlock *stackCheckMBB = MF.CreateMachineBasicBlock();
> + MachineBasicBlock *incStackMBB = MF.CreateMachineBasicBlock();
> +
> + for (MachineBasicBlock::livein_iterator i = prologueMBB.livein_begin(),
> + e = prologueMBB.livein_end(); i != e; i++) {
>
> s/i/I/ stick to coding conventions (especially within a single function).
>
Done. (also for e -> E)
> diff --git a/test/CodeGen/X86/hipe-prologue.ll
> b/test/CodeGen/X86/hipe-prologue.ll
>
<snip>
A couple of small fixes here, too; they shouldn't be a problem.
Is this OK to commit?
Best,
yiannis
--
Yiannis Tsiouris
Ph.D. student,
Software Engineering Laboratory,
National Technical University of Athens
WWW: http://www.softlab.ntua.gr/~gtsiour
-------------- next part --------------
A non-text attachment was scrubbed...
Name: hipe-prologue-v2.patch
Type: text/x-patch
Size: 11829 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130213/164c3f02/attachment.bin>
More information about the llvm-commits
mailing list