[LLVMdev] Fixing segmented stacks
Sanjoy Das
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!
--
Sanjoy Das
http://playingwithpointers.com
_______________________________________________
LLVM Developers mailing list
LLV...@cs.uiuc.edu http://llvm.cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
Jakob Stoklund Olesen
On Oct 18, 2011, at 2:02 PM, Sanjoy Das wrote:
> 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?
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
Sanjoy Das
This is exactly what I was missing. Thanks a ton! :)
--
Sanjoy Das
http://playingwithpointers.com
Jakob Stoklund Olesen
On Oct 18, 2011, at 2:33 PM, Sanjoy Das wrote:
>> it should be expanded late: In lib/Target/X86/X86MCInstLower.cpp.
>
> This is exactly what I was missing. Thanks a ton! :)
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
Anton Korobeynikov
look for "eh_return".
--
With best regards, Anton Korobeynikov
Faculty of Mathematics and Mechanics, Saint Petersburg State University
Jim Grosbach
On Oct 18, 2011, at 2:46 PM, Jakob Stoklund Olesen wrote:
>
> On Oct 18, 2011, at 2:33 PM, Sanjoy Das wrote:
>
>>> it should be expanded late: In lib/Target/X86/X86MCInstLower.cpp.
>>
>> This is exactly what I was missing. Thanks a ton! :)
>
> 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.
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
Sanjoy Das
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!
Rafael Ávila de Espíndola
> 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
Sanjoy Das
> 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.