XCore target: fix bug in XCoreLowerThreadLocal.cpp

[Robert Lytton]

Hi Rafael,

I have not looked at C++ at all viz exceptions - so thank you for bringing it to light.

I intend the default lowering to replace const expressions in an instruction with run time expressions immediately in front of the instruction. For Phi nodes, these expressions need to be placed prior to the preceding terminator instruction.

Reading the LanRef leads me to understand the 'invoke' instructions as terminator instructions, thus we can insert new expressions in front of them.
The only issue is with the return value but this is not an issue to us viz:

    There must be no non-phi instructions between the start of a basic block and the PHI instructions: i.e. PHI instructions must be first in a basic block.
    For the purposes of the SSA form, the use of each incoming value is deemed to occur on the edge from the corresponding predecessor block to the current block (but after any definition of an ‘invoke‘ instruction’s return value on the same edge).

I apologise if I am not following, and thank you for your input and interest.

Robert

________________________________________
From: Rafael Espíndola [rafael.espindola at gmail.com]
Sent: 28 September 2013 00:54
To: Robert Lytton
Cc: Richard Osborne; llvm-commits at cs.uiuc.edu
Subject: Re: XCore target: fix bug in XCoreLowerThreadLocal.cpp

I don't think this correctly handles invokes:

For the purposes of the SSA form, the definition of the value returned
by the ‘invoke‘ instruction is deemed to occur on the edge from the
current block to the “normal” label. If the callee unwinds then no
return value is available.

On 24 September 2013 05:02, Robert Lytton <robert at xmos.com> wrote:
> Hi Richard,
> The code has been changed to handle duplicate constant expressions in the
> phi instruction  (only triggers under O0).
> Robert
>
> ________________________________
> From: Richard Osborne
> Sent: 23 September 2013 18:22
> To: Robert Lytton
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: XCore target: fix bug in XCoreLowerThreadLocal.cpp
>
> Hi Robert,
>
> +          // in the calling BB, infront of the branch instruction.
>
> calling BB doesn't seem the right terminology, how about: in the
> predecessor, before the branch instruction.
>
> +          if (PHINode *PN = dyn_cast<PHINode>(WU))
> +            for (int I=0, E=PN->getNumIncomingValues(); I < E; ++I)
> +              if (PN->getIncomingValue(I) == CE) {
> +                InsertPos = &PN->getIncomingBlock(I)->back();
> +                break;
> +              }
> +          Instruction *NewInst = createReplacementInstr(CE, InsertPos);
>
> What happens if the constant expression appears twice in the phi
> instruction? Also there should be spaces around '='
>
> On 23/09/13 17:59, Robert Lytton wrote:
>
> Hi,
>
> Attached is a patch to fix:
>
>    XCore target: fix bug in XCoreLowerThreadLocal.cpp
>
>     When a ConstantExpr which uses a thread local is part of a PHI node
>     instruction, the insruction that replaces the ConstantExpr must
>     be inserted in the calling block just before the branch instruction.
>
>
> Robert
>
>
>
> --
> Richard Osborne | XMOS
> http://www.xmos.com
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>