Fixed spelling mistake in comment.

Updated some of the test cases for the 32 bit compilation.

Added the todo comment in the TD file.

Added comment for function.
Fixed variable name.
Added test case for 32 bit target.

Since D92959 is now in I am going to abandon this patch.

Forgot to delete the old lines in the TD file.

I do not have any more questions.
Thank you for refactoring this. It looks a lot cleaner.
LGTM.

I just had some minor comments. I think it makes sense overall.

Tested this on some actual legacy compiled code and it does what we want.
Only one minor question. Otherwise, LGTM.

In D92959#2449669, @MaskRay wrote:

Missing R_PPC64_TLSGD/R_PPC64_TLSGD for one function and correct for another in the same translation unit - does this only happen with relocatable links? I'd hope we simply consider this too niche to fix and don't add more logic on this...

In D92959#2449603, @stefanp wrote:

In D92959#2447405, @MaskRay wrote:

In D92959#2447003, @stefanp wrote:

Thank you for your help!

It make sense what you have written but I would still prefer to explicitly check for calls to __tls_get_addr and look at the relocations on the call.

How would you like to proceed from here? Do you want me to take the patch over and make changes?

This patch is essentially a rewrite.
If this patch looks fine, I'd like if we take this route instead....

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=727fc41e077139570ea8b8ddfd6c546b2a55627c introduced the R_PPC64_TLSGD/R_PPC64_TLSLD behavior in 2009 so assumably the problem existed before 2009. It made feel pretty uneasy that you are needing the pre-2009 workaround....

I am not sure checking "__tls_get_addr" is useful. R_PPC64_TLSGD/R_PPC64_TLSLD is only used by "__tls_get_addr". GD/LD GOT relocations are not used otherwise, are they?
So a simple check like this patch should work. We don't necessarily bring all the complexity from GNU ld.

I don't want to implement a complete fix of this. I am completely happy with just detecting the issue and disabling the relaxation if we find it.
However, if we have a scenario like this one:

Right:
  addis 3, 2, x@got@tlsgd@ha
  addi 3, 3, x@got@tlsgd@l
  bl __tls_get_addr(x@tlsgd)
  nop
  blr

Wrong:
  addis 3, 2, x@got@tlsgd@ha
  addi 3, 3, x@got@tlsgd@l
  bl __tls_get_addr
  nop
  blr

the function Wrong won't be caught by the code that you posted. We basically have two functions, one was compiled correctly and the other was not.

In the code that you have what if you just look for R_PPC64_REL24 and check those relocations for __tls_get_addr? I know that this adds more overhead but it helps with the backwards compatibility and I feel that is important. Also, if it adds too much overhead we can keep the option and that way we only do this extra check if the user asks for it.

In D92959#2447405, @MaskRay wrote:

In D92959#2447003, @stefanp wrote:

Thank you for your help!

It make sense what you have written but I would still prefer to explicitly check for calls to __tls_get_addr and look at the relocations on the call.

How would you like to proceed from here? Do you want me to take the patch over and make changes?

This patch is essentially a rewrite.
If this patch looks fine, I'd like if we take this route instead....

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=727fc41e077139570ea8b8ddfd6c546b2a55627c introduced the R_PPC64_TLSGD/R_PPC64_TLSLD behavior in 2009 so assumably the problem existed before 2009. It made feel pretty uneasy that you are needing the pre-2009 workaround....

I am not sure checking "__tls_get_addr" is useful. R_PPC64_TLSGD/R_PPC64_TLSLD is only used by "__tls_get_addr". GD/LD GOT relocations are not used otherwise, are they?
So a simple check like this patch should work. We don't necessarily bring all the complexity from GNU ld.

Thank you for the explanations!
I'm fine with keeping the if conditions looking similar and avoiding too many test changes.

Thank you for your help!

In D91611#2446779, @MaskRay wrote:

In D91611#2446331, @stefanp wrote:

In D91611#2443178, @MaskRay wrote:

I think the code should be extracted to scanRelocs: check whether there are R_TLSGD_GOT relocations without R_PPC64_TLSGD/R_PPC64_TLSLD. This may be an entire rewrite. I'm working on it.

I think I understand what you are looking for. It's my patch so I should probably be doing the rewrite myself.
I don't want you to spend more time than you have to on this...

... but do you think D92959 is complete now?

In D91611#2443178, @MaskRay wrote:

I think the code should be extracted to scanRelocs: check whether there are R_TLSGD_GOT relocations without R_PPC64_TLSGD/R_PPC64_TLSLD. This may be an entire rewrite. I'm working on it.

Rebased on top of D92089

Thank you for pointing me to this patch.

In D92879#2441581, @steven.zhang wrote:

Rebase with patch https://reviews.llvm.org/D92089 ?

Merged if statement and did a bit of cleanup according to comments.
Merged two test cases.

@MaskRay
I'm sorry to bug you but I still don't understand what you are looking for.

Gentle Ping

Added CHECK-NEXT that was missed.

Changed the multiclass into class.

A few of the lines in the TD file were too long so I fixed that.

Updated the test case to use a multiclass and added a comment to each pattern.
Also added different types to one of the patterns.

Since the scope of the patch has changed two of the tests were no longer
required so I have removed them.
Added a warning for the case where TLS relaxation is disabled due to a missing
relocation.

Updated test cases according to review.

Completely changed the way that the patch works.

I have done some testing with gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0 and GNU ld (GNU Binutils for Ubuntu) 2.30.

In D91611#2400509, @MaskRay wrote:

I remember that I have questioned about the error checking when the patch was initially brought up. The introduction of a new linker option for an incorrect case where compiler will not generate such code makes me feel more uneasy about it. Surely the linker can detect such errors but does it really have to? With assembly the user can play all sorts of tricks which will not work, but is that the duty of the linker?

In D91279#2400854, @stefanp wrote:

In D91279#2390160, @shchenz wrote:

Using dform with offset 0 can save one register r0/X0, this is benefit for register allocation? But adding it in PPCPreEmitPeephole pass which is after register allocation will make the benefit gone.
Maybe we need to do it before register allocation? For example at the place where the x-form with zero register is generated.

I checked one example loadConstant in test/CodeGen/PowerPC/f128-passByValue.ll.
We generate LXVX $zero8, in ISEL because we meet the worst case and we don't have d-form choice for the instruction selection. so we have to use x-form and in x-form selection, we have to use zero/zero8 as the base and use load address as the index. See PPCTargetLowering::SelectAddressRegRegOnly.

I guess most cases are with same reason for generating x-form + zero register, we meet the worst case in ISEL, so we have to use x-form + zero register form, with this form, we can always select a powerpc load/store instruction.

For me, a better solution should be change the worst case handling in ISEL, it is before RA and it is also transparent for types like STXVX/LXVX/ and also LDX/STDX, LFDX/STFDX...

I'm just going to jump in to give a little more background. The initial reason we wanted to do this was to enable an optimization that actually happens in the linker after the code is emitted.
To get the idea you can look at this test:

/llvm/test/CodeGen/PowerPC/pcrel-linkeropt.ll

Which contains this section:

; FIXME: we should always convert X-Form instructions that use
; PPC::ZERO[8] to the corresponding D-Form so we can perform this opt.
define dso_local void @ReadWrite128() local_unnamed_addr #0 {
; CHECK-LABEL: ReadWrite128:
; CHECK:       # %bb.0: # %entry
; CHECK-NEXT:    pld r3, input128@got@pcrel(0), 1
; CHECK-NEXT:    lxvx vs0, 0, r3
; CHECK-NEXT:    pld r3, output128@got@pcrel(0), 1
; CHECK-NEXT:    stxvx vs0, 0, r3
; CHECK-NEXT:    blr
entry:
  %0 = load i128, i128* @input128, align 16
  store i128 %0, i128* @output128, align 16
  ret void
}

When we have a GOT access like this it is possible for the compiler to mark the instruction with R_PPC64_PCREL_OPT and then the linker merges the two instructions into one and replaces the second instruction with a nop. The problem is that this opt can only be done if the second instruction is DForm. We noticed that when we implemented this optimization we could not catch all of the cases because in some situations (like the one above) we use the XForm instead of the DForm.

Having said that, we should try to do this before the PreEmitPeephole. The optimization that adds the R_PPC64_PCREL_OPT relocation is also in the PreEmitPeephole and I'm not sure if it will be detected if we do both things at the same time (both as in convert the XForm to a DForm and then have the same opt use that DForm to add the relocation).

I agree that ISel is a better place for this. If we cannot do this in ISel then we should still try to do this before we get to the PreEmitPeephole or at least make sure that both the DForm is present and that the R_PPC64_PCREL_OPT relocation is added as we expected in the same pass.

In D91279#2390160, @shchenz wrote:

Using dform with offset 0 can save one register r0/X0, this is benefit for register allocation? But adding it in PPCPreEmitPeephole pass which is after register allocation will make the benefit gone.
Maybe we need to do it before register allocation? For example at the place where the x-form with zero register is generated.

I checked one example loadConstant in test/CodeGen/PowerPC/f128-passByValue.ll.
We generate LXVX $zero8, in ISEL because we meet the worst case and we don't have d-form choice for the instruction selection. so we have to use x-form and in x-form selection, we have to use zero/zero8 as the base and use load address as the index. See PPCTargetLowering::SelectAddressRegRegOnly.

I guess most cases are with same reason for generating x-form + zero register, we meet the worst case in ISEL, so we have to use x-form + zero register form, with this form, we can always select a powerpc load/store instruction.

For me, a better solution should be change the worst case handling in ISEL, it is before RA and it is also transparent for types like STXVX/LXVX/ and also LDX/STDX, LFDX/STFDX...

In D85994#2350718, @MaskRay wrote:

In D85994#2350659, @stefanp wrote:

@MaskRay
Sorry... can you pause on this for a second. I want to talk to the glibc guys about this first.

@stefanp I already did it... I think this is a problem for many ld.so implementations. For example, musl has

if ((def.sym->st_info&0xf) == STT_TLS)
         return __tls_get_addr((tls_mod_off_t []){def.dso->tls_id, def.sym->st_value-DTP_OFFSET});

It was broken as well...

@MaskRay
Sorry... can you pause on this for a second. I want to talk to the glibc guys about this first.

In D85994#2350648, @MaskRay wrote:

In D85994#2350641, @stefanp wrote:

@MaskRay
Would you like me to just revert this change? Do you want to see something on Phabricator for it?

I have prepared a partial revert, as the test is still useful. I can do it if you allow:)

@MaskRay
Would you like me to just revert this change? Do you want to see something on Phabricator for it?

In D85994#2350431, @MaskRay wrote:

In D85994#2219062, @MaskRay wrote:

First, if this issue is not common enough, I'd rather we don't have a diagnostic at all.

If we do need it, note that *(i - 2); may be out of bounds.

Last, we should avoid pre-built object files. You can synthesize them with yaml2obj. The preferred style is llvm/test/tools/llvm-readobj/ELF/*.test. lld/test/ELF has some yaml2obj tests as well.

@stefanp The error turns out to be a problem when building glibc.

elf/dl-sym.c has a raw __tls_get_addr call (it sets up the parameter by itself), without R_PPC64_TLSGD/R_PPC64_TLSLD (similar to CallOnly). Shall we drop the logic? I believe GNU ld does not have the logic.

/* Return the symbol address given the map of the module it is in and
   the symbol record.  This is used in dl-sym.c.  */
static void *
_dl_tls_symaddr (struct link_map *map, const ElfW(Sym) *ref)
{
# ifndef DONT_USE_TLS_INDEX
  tls_index tmp =
    {
      .ti_module = map->l_tls_modid,
      .ti_offset = ref->st_value
    };

  return __TLS_GET_ADDR (&tmp);
# else
  return __TLS_GET_ADDR (map->l_tls_modid, ref->st_value);
# endif
}
#endif

Updated comment in test case.

This is still causing a buildbot failure on a Power PC sanitizer bot:
http://lab.llvm.org:8011/#/builders/18

Fixed the LLVM_FALLTHROUGH
Added split-file and -soname to the test file.

Fixed a couple of comments.
Moved the R_PPC64_DTPREL34 case first and adjusted the dynamic thread offset
there before falling through to the common case.

Fixed the issue where the 0x8000 offset was not being considered.

LGTM

Added the missing header file.
Added the license comment.

Related to this please see Nemanja's comment on https://reviews.llvm.org/D82485.

I can confirm that this changeset is causing the timeout on the clang-ppc64be-linux buildbot.
The following was run on a Big Endian Power PC machine.

All of the dependencies of this patch have now been committed.

Rebased to top of trunk.
Fixed a couple of comments.
Removed some lines that are not required form the test.

Ok, thank you MaskRay.
Sorry, in my head I didn't put the error together with what you had said previously.

Looks like .rela.dyn is located in different places depending on the machine.
From two different bots:

# GDTOIE-RELOC: Relocation section '.rela.dyn' at offset 0x10118 contains 2 entries:
                ^
<stdin>:2:1: note: scanning from here
Relocation section '.rela.dyn' at offset 0x10128 contains 2 entries:

or

# GDTOIE-RELOC: Relocation section '.rela.dyn' at offset 0x10118 contains 2 entries:
                ^
<stdin>:2:1: note: scanning from here
Relocation section '.rela.dyn' at offset 0x10148 contains 2 entries:

Reopening this to see why the test failed.

Updated the comment.

Further reduced the test case.

Removed the last function from the test assembly file.

LGTM