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

[Dan Gohman]

On Apr 5, 2009, at 5:41 AM, Rafael Espindola wrote:

>> I think I will try the idea of always lowering to (load gs:0) + ADDR
>> and then implementing some instruction folding after Select. I assume
>> that is possible, right?
>
> The attached patch does that. I like it a lot more. All that was
> needed to get performance back was a small change to MatchAddress to
> handle a load inside a load.
>
> The only possible issue is that now we produce
>
> movl    $i at NTPOFF, %eax
> addl    %gs:0, %eax
>
> Instead of
>
> movl       %gs:0, %eax
> leal       i at NTPOFF(%eax), %eax
>
> I don't have enough experience with X86 to know which one is best.

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.

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

>
>
> What do you think of the current approach?

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.

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

Please use spaces instead of tabs for indentation.

Thanks for working on this!

Dan