Slight fly in the ointment: The existing tests exposed that that this implementation is too naive and is renaming things it shouldn't. I'll be back with a more mature implementation. Thanks for the reviews and patience

TODO: Testcase

In your commit message:

• s/parametrs/parameters

Merged in https://reviews.llvm.org/rG940fa35ece5294a115a2fdba89ef6c095d90df0f wherein I forgot to note the differential revision

Thanks. LGTM

> Index: llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp
> ===================================================================
> --- llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp
> +++ llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp
> @@ -4326,8 +4326,13 @@
> const uint64_t ABITypeAlign = DL.getABITypeAlign(ArgTy).value();
>  
> // If a function has linkage different from internal or private, we
> - // must use default ABI alignment as external users rely on it.
> - if (!(F && F->hasLocalLinkage()))
> + // must use default ABI alignment as external users rely on it. Same
> + // for a function that may be called from a function pointer.
> + if (!F || !F->hasLocalLinkage()) ||
> + F->hasAddressTaken(/*Users=*/nullptr,
> + /*IgnoreCallbackUses=*/false,
> + /*IgnoreAssumeLikeCalls=*/true,
> + /*IngoreLLVMUsed=*/true))
> return Align(ABITypeAlign);
>  
> assert(!isKernelFunction(*F) && "Expect kernels to have non-local
> linkage");

s/IngoreLLVMUsed/IgnoreLLVMUsed/g
otherwise LGTM

Code review suggestions

Do you have commit access?

LGTM

In D111315#3048459, @v.g.vassilev wrote:

Can you map state_not_recoverable to not_supported?

In D99334#2702227, @dblaikie wrote:

In D99334#2701760, @ldrumm wrote:

Why would code handling llvm.dbg.cu need to add an if statement? I'd expect it to walk through whatever's in llvm.dbg.cu and if there's nothing it'd be fine with that?

Because 3 lines later we have auto *NMD = NewModule->getOrInsertNamedMetadata("llvm.dbg.cu");
In the case that the source module has no llvm.dbu.cu the target module ends up with on without operands. This is both illegal and a pain to deal with in user code.

I don't understand why it would be either illegal or a pain to deal with. (& I'm not sure it's illegal either - I modified an example IR file to have an empty llvm.dbg.cu list and it seemed to compile file (compared to other violations of IR debug info verification, which did result in an error/warning like this:

inlinable function call in a function with debug info must have a !dbg location
  call void @_Z2f1v()
warning: ignoring invalid debug info in test.ll
```)

Why would code handling llvm.dbg.cu need to add an if statement? I'd expect it to walk through whatever's in llvm.dbg.cu and if there's nothing it'd be fine with that?

ping

In D99334#2659955, @dblaikie wrote:

what was invalid about the metadata? The llvm.dbg.cu list was empty? I wonder if we should just say that's valid. It seems pretty benign/reasonable - makes it easier to write code like this, or code that, say, stripped a CU out wouldn't have to worry about removing a potentially zero-length cu list. @aprantl @JDevlieghere ?

kito-cheng would you mind looking at the testing here and see whether the correctness testing of NaNs is sufficient?

In D98670#2649998, @kito-cheng wrote:

I just found an issue is unrelated this patch, but related to fp16, RISC-V GCC using the traditional libgcc function name scheme like __extendhfdf2(__<op><srcT><dstT><N_OP>) rather than __gnu_f2h_ieee (__gnu_*2*_ieee).

Function used in GCC:

• __extendhfsf2 for half -> float
• __truncsfhf2 for float -> half
• __extendhfdf2 for double -> half
• __truncdfhf2 for half -> double

In D98957#2643442, @dexonsmith wrote:

Thanks for cleaning this up!

Mostly LGTM, just please split this up into three separate commits (no need for new reviews; any order seems fine, as they seem fully independent):

Good idea. Will do

• Unrelated whitespace changes (BTW, if you just got these incidentally, you can use git-clang-format in the future to just change the lines your commit is touching...).

Seeing as there is now a whitespace only commit, I've just opted to format the whole file as there are plently of other formatting issues picked up in this file

In D98411#2628592, @Anastasia wrote:

In D98411#2626613, @ldrumm wrote:

Given this fixes a runtime crash bug, should it be backported to the relevant release branch?

Sure, thanks!

Given this fixes a runtime crash bug, should it be backported to the relevant release branch?

@Anastasia @svenvh is this already covered by https://reviews.llvm.org/D98039 ?

Ping

Patch updated to not insert trailing blank

I absent mindedly responded to this review via email, without actually checking whether Phabricator supports this. Apologies for the delay

included full diff context

An example of the output:

I found similar unittests for other DWARF entries which allow me to check more about the parser state, so I went with Pavel's suggestion for the testcase

Now emit long-string disabling GCC diagnostic pragmas around string definitions to silence gcc's -Woverlength-strings on a case-by-case basis

• only pass the --long-string-literals flags to *LLVM* tablegen (fixes clang-tablegen problem)
• suppress overlenth-string-literals warning seen in the wild: this behaviour is intended and shouldn't be warned against

In D73044#1836244, @broadwaylamb wrote:

Hello @ldrumm,

Sorry, I had to revert this commit in e464b31c1565204e3be114d043bcbf4de61fe2e9, because it broke some Windows builds:
http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/13870

Remove compile-time detection. Make this a runtime only flag

In D73044#1831436, @mstojanovic wrote:

First char of the title: { -> [.

Thanks. Phabricator doesn't seem to track the git commit message, but I've fixed this is on my tree

Change the cmake flag check to if(MSVC) rather than if(CMAKE_GENERATOR MATCHES "Visual Studio"). CMAKE_GENERATOR is not the compiler

Ensure elaborate cross-compilation setups are supported