[PATCH][X86] Trying to fix PR20243 - leaf frame pointer bug on X86, ELF, PIC

[Reid Kleckner]

Works for me, I think that definitely fixes a bug in existing code. Rafael,
do you think this approach is OK?

On Tue, Nov 11, 2014 at 7:49 AM, Dario Domizioli <dario.domizioli at gmail.com>
wrote:

> Good news! (maybe)
>
> I was focusing too much on the instruction definition and not enough on
> looking at how the instruction was getting created...
> While I inspected the ISel lowering code creating the TLS access pseudo
> (in X86ISelLowering.cpp) to find out how to do something similar to what
> AArch64 does, I found a comment in the GetTLSADDR() function basically
> saying that the code there was responsible for informing the
> MachineFrameInfo that the function had calls... however it was only setting
> the "adjustsStack" flag and not the "hasCalls" flag.
>
> This means that I don't strictly need to change the definition of the TLS
> access pseudo, and I can fix the PR with just a one-line change in
> X86ISelLowering.cpp.
>
> Do you think it's acceptable?
> I attach the new patch. I also changed the comment in the test to explain
> the situation.
>
> Cheers,
>     Dario Domizioli
>     SN Systems - Sony Computer Entertainment Group
>
>
>
>
>
>
>
>
>
> On 11 November 2014 12:16, Dario Domizioli <dario.domizioli at gmail.com>
> wrote:
>
>> Thanks Rafael!
>>
>> I'll have a look at the AArch64 version (and the PPC patch) and I'll try
>> to implement something similar.
>> I'll post a new patch soon.
>>
>> Cheers,
>>     Dario Domizioli
>>     SN Systems - Sony Computer Entertainment Group
>>
>>
>>
>>
>>
>> On 11 November 2014 05:45, Rafael Espíndola <rafael.espindola at gmail.com>
>> wrote:
>>
>>> For a related issue, see http://reviews.llvm.org/D6209
>>>
>>> On 10 November 2014 16:02, Rafael Espíndola <rafael.espindola at gmail.com>
>>> wrote:
>>> > On 10 November 2014 10:48, Dario Domizioli <dario.domizioli at gmail.com>
>>> wrote:
>>> >> Hi Rafael.
>>> >>
>>> >> I have been looking at this for a while, but I cannot seem to find a
>>> way of
>>> >> preserving an LLVM intrinsic after ISel and expanding it later.
>>> >> Basically, if I add an LLVM intrinsic for TLS access in
>>> IR/IntrinsicsX86.td,
>>> >> then the backend expects me to provide an instruction selection rule
>>> for
>>> >> lowering it to MachineInstructions; however as you said we cannot do
>>> it at
>>> >> ISel stage because we must ensure the structure is preserved so that
>>> the
>>> >> linker can pattern-match it.
>>> >>
>>> >> With "intrinsic", do you instead mean an actual function that we
>>> manufacture
>>> >> on the fly in Clang (or provide a declaration for in a header), and
>>> then
>>> >> eliminate / patch up in MC later on?
>>> >> That might work but it seems a bit convoluted to me. Also... if we
>>> >> manufacture the function, we end up with a declaration in the IR that
>>> >> doesn't correspond to anything in the source; if we instead have the
>>> >> function in a header, doesn't that require the user to include the
>>> header?
>>> >> Is that what you're suggesting, or am I getting confused?
>>> >
>>> > Sorry, you were right. This has to be an instruction, it is too late
>>> > to have an intrinsic.
>>> >
>>> > Your change to add a isCall is probably also correct. The problem with
>>> > TLS_addr64 is that it has not been upgraded to use the new call
>>> > representation with register masks.
>>> >
>>> > Probably the best example of how this should work is what AArch64 does:
>>> >
>>> > -----------------------------------------------------------------------
>>> > let isCall = 1, Defs = [LR] in
>>> > def TLSDESC_BLR
>>> >     : Pseudo<(outs), (ins GPR64:$dest, i64imm:$sym),
>>> >              [(AArch64tlsdesc_call GPR64:$dest, tglobaltlsaddr:$sym)]>;
>>> >
>>> ------------------------------------------------------------------------
>>> >
>>> > so it has isCall, but not the explicit Defs list. It is created with
>>> >
>>> > -------------------------------------
>>> >   const uint32_t *Mask = ARI->getTLSCallPreservedMask();
>>> > ....
>>> >
>>> >   SmallVector<SDValue, 6> Ops;
>>> >   Ops.push_back(Chain);
>>> >   Ops.push_back(Func);
>>> >   Ops.push_back(SymAddr);
>>> >   Ops.push_back(DAG.getRegister(AArch64::X0, PtrVT));
>>> >   Ops.push_back(DAG.getRegisterMask(Mask));
>>> >   Ops.push_back(Glue);
>>> >
>>> >   SDVTList NodeTys = DAG.getVTList(MVT::Other, MVT::Glue);
>>> >   Chain = DAG.getNode(AArch64ISD::TLSDESC_CALL, DL, NodeTys, Ops);
>>> > --------------------------------------
>>> >
>>> > With this X86FloatingPoint.cpp should not get confused about
>>> > __tls_get_addr returning a value in the fp stack.
>>> >
>>> > Cheers,
>>> > Rafael
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20141111/944e47c0/attachment.html>