I have replied to the comments and will update the code accordingly. Some of the codes are intentionally left to be update in 5.1 implementation. Will add TODO comments there.
I will revisit the implementation of getBestWhenMatchForContext and also add more test cases over the weekend and submit new code by next week.

In D91611#2417611, @stefanp wrote:

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.

It appears that ld.bfd by default fixes up the relocation if it is missing and then does the relaxation correctly. I don't need to specify any options for this to be done.
As a simple example:

Test1:
.LCF0:
  addis 2,12,.TOC.-.LCF0@ha
  addi 2,2,.TOC.-.LCF0@l
  .localentry     Test1,.-Test1
  addis 3,2,x@got@tlsgd@ha
  addi 3,3,x@got@tlsgd@l
  bl __tls_get_addr
  nop

The bl __tls_get_addr is missing the relocation ld.bfd still relaxes it correctly.
However, with a more complex example that I wrote by hand:

Test1:
.LCF0:
  addis 2,12,.TOC.-.LCF0@ha
  addi 2,2,.TOC.-.LCF0@l
  .localentry     Test1,.-Test1
  addis 3,2,x@got@tlsgd@ha
  addi 3,3,x@got@tlsgd@l
  addis 3,2,x@got@tlsgd@ha
  addi 3,3,x@got@tlsgd@l
  bl __tls_get_addr
  nop

in the above case ld.bfd does not recognize the pattern for the TLS and so it turns off TLS relaxations completely.
The reason I brought up these examples is because I feel that detecting the difference between the two cases in lld would be unreasonably complicated an probably not worth it. As a result I feel that it may not be feasible to do the same thing that ld.bfd is doing.

Here is my new proposal:
We can add an option to lld. Unfortunately this option will not exist in ld.bfd because they wouldn't need it.
The option is off by default.
If the option is off then lld will behave exactly the same way that it always has. No error detection for missing relocations.
If the option is on then lld will check all calls to __get_tls_addr and if it finds a missing relocation it will turn off TLS relaxations. It won't try to add the missing relocation and it won't throw an error.
This design will not cause problems to default behaviour (and it shouldn't add overhead to the default link either) but it will also allow linking with code that was generated with the legacy compiler if the user specifies the option.

How does this new idea sound?
If everyone agrees on this idea (or a variant of this idea) I will rework the patch like this.

In D91611#2417611, @stefanp wrote:

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.

It appears that ld.bfd by default fixes up the relocation if it is missing and then does the relaxation correctly. I don't need to specify any options for this to be done.
As a simple example:

Test1:
.LCF0:
  addis 2,12,.TOC.-.LCF0@ha
  addi 2,2,.TOC.-.LCF0@l
  .localentry     Test1,.-Test1
  addis 3,2,x@got@tlsgd@ha
  addi 3,3,x@got@tlsgd@l
  bl __tls_get_addr
  nop

The bl __tls_get_addr is missing the relocation ld.bfd still relaxes it correctly.
However, with a more complex example that I wrote by hand:

Test1:
.LCF0:
  addis 2,12,.TOC.-.LCF0@ha
  addi 2,2,.TOC.-.LCF0@l
  .localentry     Test1,.-Test1
  addis 3,2,x@got@tlsgd@ha
  addi 3,3,x@got@tlsgd@l
  addis 3,2,x@got@tlsgd@ha
  addi 3,3,x@got@tlsgd@l
  bl __tls_get_addr
  nop

in the above case ld.bfd does not recognize the pattern for the TLS and so it turns off TLS relaxations completely.
The reason I brought up these examples is because I feel that detecting the difference between the two cases in lld would be unreasonably complicated an probably not worth it. As a result I feel that it may not be feasible to do the same thing that ld.bfd is doing.

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.

Thanks! Update the commit as review comments.

In D92080#2416625, @craig.topper wrote:

gcc calls the *l version with -mlong-double-128 on x86. Should we match gcc here?

Added regex and variable for function attribute

Actually, it only means that isLoopBackedgeGuardedByCond should directly call isKnownPredicateAt(Latch->getTerminator()) as the last resort. I will follow-up with this improvement.

Eeek. I found a test where isKnownPredicateAt is more powerful. Need to use both then.

True but orthogonal. This will raise a link error on libomptarget.rtl.cuda.so if it has symbols that are not resolved by the dynamic libraries it is linked against. E.g. amdgpu links with this and doesn't yet implement the device to device copy function.

Updated unit test

rebase

In D71124#2415424, @oceanfish81 wrote:

I tried to build the following:

https://gist.github.com/advancedwebdeveloper/09033869445a09a2e297a73387d46c41/raw/bf7dac33e256621c445f9e61f88a832e50d901a3/llvm_cpu_features_investigation.cpp

but it didn't show me any CPU features, on https://fedoraproject.org/wiki/Architectures/RISC-V/Installing#Boot_under_QEMU

feedback

(reviving an old discussion) to summarize the issue:
The issue was whether it is okay to make dereferenceable %p non-UB if %p was not dereferenceable.

Patch D92089 can cover this.

The patch D92089 can cover this.

@djasper gental ping~

Address review comment

In D91935#2415346, @tstellar wrote:

Thanks for the explanation. Doing a git clone as part of the build is usually discouraged, though I see Lang approved the original patch. Is this code disabled by default?

This is a nice change, but not all plugins can implement all interfaces listed in exports. For example, if the plugin doesn't support async operations, then those interfaces suffix with _async will not be implemented by the plugin, and libomptarget can acknowledge that because dlopen returns nullptr.

This needs an implementation for x86_64 offloading or a change to the test to only run on nvptx. Intend to do the former.

1 comment, but otherwise LGTM.

Had a long conversation w/Florian today; he was nice enough to give a lot of context and general direction which helped me understand where this is going. It also helps that e0c479cd, and 00a6601 landed recently showing alignment with the direction sketched here.

Removed duplicate tests as suggested by leonardchan

LGTM. Thanks

LGTM.

Thanks for catching this. LGTM

Thank you, Aart!

FYI, I went ahead and reverted this along with its dependent change since we didn't hear back.

Thanks, looks good

The best I can tell the underlying OpenMP problem is the lookup code in buildUserDefinedMapperRef. In this code if there is no mapper-id (like this test) the name is changed to "default" but the Loc is not set. If we set the location to something (StartLoc is handy here) all is well.

Add test; add warning in lld when it sees empty subsection

LGTM.

FYI, this patch (unintentionally?) fixed a miscompile for us, as I noted in the review thread for D90648. Reposting here for visibility:

Well, I *can* think of a way to test this from a .s file. Instead of using .cv_inline_line_table, you could use a sequence of .byte 0xNN, 0xNN directives. I think you can get the hex codes from llvm-pdbutil dump -symbols. How about it?

LGTM

(I marked this as depending on D92140 because if you apply this patch to the current HEAD directly, the tests will break.)