Fixing segmented stacks

Hi!

First of all, sorry for the delay.

This about fixing the issue with having a the BB ending with a
non-terminating instruction when compiling with segmented stacks. I'm
not sure if having an isel pseudo instruction which is lowered into a
RET and then a MOV would work better.
LLVMTargetMachine::addCommonCodeGenPasses adds the
ExpandISelPseudosPass before the PEI pass (so it boils down to the
same thing, I think -- the verifier will still complain). This is, of
course, assuming I understood Duncan correctly.

The only fix I can think of is splitting the thing into two separate
basic blocks. I already have sent Duncan patches which does this. Of
course, then I'll have a BB that ends with a RET, but has successors.
As it was pointed out, this may not be desirable.

Is there some third way, better way? I think the best solution would
involve delaying the addition of this extra bit of code till the very
last moment. How about adding something to
X86TargetMachine::addPreEmitPass? Will that work?

Thanks!

If I understand correctly, you need to insert:

  call _morestack
  ret

in the middle of a basic block. There are two problems:

1. RET is a terminator, and can only appear at the end of a basic block.
2. The CALL and RET instructions must be adjacent.

The best way to handle this is to create a pseudo-instruction representing the call+ret pair.

Since the two instructions must stay together, it should be expanded late: In lib/Target/X86/X86MCInstLower.cpp.

/jakob

it should be expanded late: In lib/Target/X86/X86MCInstLower.cpp.

This is exactly what I was missing. Thanks a ton! :slight_smile:

We have three pseudo expansion passes:

1. ExpandISelPseudos.cpp - For instructions that may need to create basic blocks, like CMOV and atomics.

2. ExpandPostRAPseudos.cpp - For instructions used to trick the register allocator into doing the right thing, and COPY instructions created by live range splitting.

3. *MCInstLower.cpp - For instructions that need to trick all of codegen.

Pseudos should be expanded as early as possible. Many of the instructions currently expanded in X86MCInstLower could be moved to the PostRA expansion pass. That would also allow them to be converted into pure isPseudo=1 instructions instead of just isCodeGenOnly=1.

/jakob

The best way to handle this is to create a pseudo-instruction representing the call+ret pair.

There is already and example of somehow similar pseudo. One should
look for "eh_return".

FWIW, even those expanded at MCLowering time can be pure pseudos. There is no need to use isCodeGenOnly definitions for any new code. Those that exist are pure legacy.

-Jim

Hi!

The first patch fixes the problem of a MOV after a RET by emitting a
fake instruction (as suggested by Duncan), which is lowered in
MCInstLower.

The second patch fixes a bug reported by -verify-machineinstrs.

Thanks!

0001-Use-a-fake-instruction-for-the-stack-expansion-seque.patch (5.15 KB)

0002-Fixes-an-issue-reported-by-verify-machineinstrs.patch (4.2 KB)

Hi!

The first patch fixes the problem of a MOV after a RET by emitting a
fake instruction (as suggested by Duncan), which is lowered in
MCInstLower.

The second patch fixes a bug reported by -verify-machineinstrs.

Do you want to add -verify-machineinstrs to the test?

Your patch looks good to me. I will commit it tomorrow if no one objects.

Thanks!

Thanks,
Rafael

Hi Rafael!

Do you want to add -verify-machineinstrs to the test?

Yes, it sounds like a very good idea. Let me know if I should re-roll
the series -- I did not want to increase clutter for such a small
change.

Thanks!

add-verify-machineinstrs (717 Bytes)