[llvm-commits] [llvm] r66922 - in /llvm/trunk: lib/Target/X86/X86InstrInfo.td test/CodeGen/X86/tls13.ll test/CodeGen/X86/tls14.ll

[Rafael Espindola]

> It probably depends on the context. The latter at least has the
> advantage of allowing the load from %gs:0 of being CSE'd when
> there are multiple TLS address computations.
>
> Can this be fixed with code in
> X86DAGToDAGISel::IsLegalAndProfitableToFold ? The code at the
> top of the function is a heuristic with a similar purpose.

Yes. Doing a "ret 1" makes we try to use a lea.

> In any event, I don't consider this a show-stopper; we can
> continue to tune things like this after the patch is in.

OK. Will update the tests and commit.

> I think this patch looks like a good approach. I think there are
> some tuning things that can be addressed later; I just have a few
> minor comments for now.
>
> In this code in isMem in X86InstrInfo.h:
>
> @@ -256,6 +256,7 @@
>    return Op+4 <= MI->getNumOperands() &&
>      MI->getOperand(Op  ).isReg() && isScale(MI->getOperand(Op+1)) &&
>      MI->getOperand(Op+2).isReg() &&
> +    MI->getOperand(Op+4).isReg() &&
>      (MI->getOperand(Op+3).isImm() ||
>       MI->getOperand(Op+3).isGlobal() ||
>       MI->getOperand(Op+3).isCPI() ||
>
> The code here only checks for getNumOperands being >= Op+4, so the
> call to getOperand(Op+4) could potentially go out of bounds. A simple
> fix would be to create a separate isLeaMem function which only expects
> 4 operands so that isMem can expect 5 (or X86AddrNumOperands)
> operands. A more involved fix would be to make all addresses have 5
> operands, and just require the fifth operand in an lea to be %reg0.
> I don't have a strong preference at this point.

I added isLeaMem.

> Please add a comment in MatchLoad explaining why it's safe to
> do what it's doing.

Done.

> Please use spaces instead of tabs for indentation.

Done.

> Thanks for working on this!

Thanks for the tip on how to fix this.

> Dan
>

Cheers,
-- 
Rafael Avila de Espindola

Google | Gordon House | Barrow Street | Dublin 4 | Ireland
Registered in Dublin, Ireland | Registration Number: 368047