[PATCH] D120000: [1/2] TLS loads opimization (hoist)

[Xiang Zhang via Phabricator via llvm-commits]

xiangzhangllvm added a comment.

In D120000#3358477 <https://reviews.llvm.org/D120000#3358477>, @efriedma wrote:

> In D120000#3357639 <https://reviews.llvm.org/D120000#3357639>, @nlopes wrote:
>
>> In D120000#3357413 <https://reviews.llvm.org/D120000#3357413>, @craig.topper wrote:
>>
>>>> Anyway, this transformation works by creating some artificial bitcasts that you expect to be carried over to the backend so the lowering can share the calls to the TLS function.
>>>>
>>>> This strategy seems very brittle to me. If some later optimization decides to remove the bitcast, your optimization will stop working. It's very likely that will be the case once opaque pointers take over.
>>
>> Didn't know there was a precedent of using this technique. Seems fragile to me, but...
>
> This is running late enough in the pipeline that it's probably fine.  Optimization passes that are part of of the codegen pipeline, like ConstantHoisting/CodeGenPrepare/etc. have an implicit contract with each other and SelectionDAG to make this sort of thing work.

Yes, @nlopes , I think efriedma  has answered your question, and that is why I put the TLSHoist pass at the late pass of mid-end.

> Really, this transform should handled by some sort of general LICM, but we can't use IR LICM because the operations aren't visible in IR, and we can't use MachineLICM because we can't really hoist calls after isel.  (Making MachineLICM handle this isn't impossible, but it gets messy because you're dealing with target-specific call sequences.)

Yes, that is right, but for MIR, it still can not see the call func (__tls_get_addr), MIR just mark the TLS with a flag, some like:

  TLS_base_addr64 $noreg, 1, $noreg, target-flags(x86-tlsld) @thl_x, $noreg,   ...

And MachineLICM only focus on Loops, current patch want to handle the duplicated call in all the function.

>>> Is this much different than the artificial bitcast we use in ConstantHoisting?
>
> More generally, the current representation of constants in IR isn't ideal, but improving constant expressions isn't really anyone's priority at the moment.  Maybe someone will look at it once the opaque pointers are done.
> Would it make sense to do this transform as part of ConstantHoisting, instead of as a separate transform?  Most of the necessary infrastructure is already there.

In fact, I though about it when I begin do this job, their logic is similar. The main reason I didn't go such way (integrate them) is

1. I want to let TLS hoist pass more later than it.
2. I want to keep ConstantHoisting is pure to handle Constant (In fact, TLS is not constant).
3. Let TLSHoist be simple and easy to control (enable or disable it).

> I don't understand why you'd want a clang flag to control this. The backend knows when it's going to generate a call to get the tls pointer.  And when we generate a call, hoisting that call is pretty obviously profitable.

Because I want to add an clang option for it. So I provide such a function attribute (flag) to let clang option "control" it.
May be this is not good idea, but I think that is another independent topic, I can refine it in [2/2] patch.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D120000/new/

https://reviews.llvm.org/D120000