In D124085#3470201, @craig.topper wrote:

In D124085#3461526, @kovdan01 wrote:

As for now, the patch contains no tests. The reason is that a minimal test lasts unacceptably long (didn't manage to wait until end) with assertions enabled - the bottleneck is checkForCyclesHelper function. With expensive checks enabled, one more bottleneck appears: DAGTypeLegalizer::PerformExpensiveChecks. When commenting those functions bodies, minimal test with release-checked compiler lasts for about 160s on my PC. Could anyone suggest a way to test this patch correctly? IMHO looks like we can leave it without special test because the fix is pretty trivial, while I personally see no obvious way to test it.

Since this in CodeGenPrepare, couldn't you write a test that uses opt -codegenprepare and not go through SelectionDAG?

Would be glad if someone could review this. Thanks!

That fixed my issue for X86, thanks! Regarding other targets - it turns out that my patch gives no performance enhancement (tested that). So abandon the revision - as for now it seems not needed anymore.

UPD: fixed the problem, keep the comment for history.

@lattner Hello Chris, could you please suggest a reviewer for this patch? I'm not sure who is the owner of this part of repository. Thanks!

In D120714#3432615, @thakis wrote:

+1 to craig.topper's point. It's true that this is what ABI_BREAKING_CHECKS is for, but we should only use this if it saves significant memory. This uint16_t lives next to two bools, so removing it doesn't save any memory (it'll just become padding) as far as I can tell.

Have you seen an actual memory use improvement from this?

As far as I can tell, this adds a bunch of complexity without a benefit (?)

@dexonsmith Please let me know if you see some issues in the patch. Thanks!

I’m currently working on a patch solving a similar problem (actually, an inseparable chain of problems).

Fix broken tests for targets not covered by the patch previously

In D120714#3417416, @dexonsmith wrote:

Hmm, the tests look much harder to read now. I'm not sure that's better.

Seems simpler/cleaner to use REQUIRES: asserts, abi-breaking-checks.

In D120714#3400246, @dexonsmith wrote:

Thanks for the pings! I'd missed your updates.

In D120714#3377826, @kovdan01 wrote:

@dexonsmith Thanks for your comments! Decided to choose the last approach that you mentioned - keep and use PersistentId regardless build type or any macro. The problem with #if LLVM_ENABLE_ABI_BREAKING_CHECKS is testing with LLVM_ABI_BREAKING_CHECKS set to FORCE_OFF during initial CMake configuration - some tests rely on PersistentId. I suppose that having PersistentId present in all builds is definitely worth small CPU overhead compared to the current state.

Which tests are you talking about? Can those tests be marked as UNSUPPORTED when ABI-breaking checks are off?

If not, why not?

It would be good to add a comment to the sources explaining why LLVM_ENABLE_ABI_BREAKING_CHECKS can't/shouldn't be used for PersistentId, and potentially a FIXME with how to fix the blockers such as they are. Probably next to the declaration of the field.

[CodeGen] Place SDNode debug ID declaration under appropriate #if

@jdoerfert Merged, hope your CI will be fixed. Could you please change "Changes requested" mark in D120129 when you verify that everything is OK? Just to make that revision closed again.

In D120129#3410514, @jdoerfert wrote:

Our internal build bots and CI for some projects are broken for 3 days. I wish to unbreak them so we get actual meaningful results, e.g., see if something else is breaking our build. I'm fine with a separate patch on top but I would prefer it now so people can run code on Monday.

In D120129#3410510, @jdoerfert wrote:

The two assertions introduced here do not hold for the libdevice.bc above. So whenever we link the above we will cause the assertions to fail. That's what I mean with break. isKernelFunction is probably going to fix all that. For a test, copy annotations like the above into one of the .ll files and make sure it contains a private/internal function w/ arguments as well.

In D120129#3410479, @jdoerfert wrote:

Please revert this commit. It breaks any code using the cuda.11.0.2 libdevice.bc file, the source of the null nvvm.annotations and the annotations with 5 arguments. See below.
Once a fix is in, and a test is added, we can reapply this.

$ opt -S .../cuda-11.0.2/nvvm/libdevice/libdevice.10.bc | tail
!nvvm.annotations = !{!1, !2, !1, !3, !3, !3, !3, !4, !4, !3}

!0 = !{i32 1, i32 4}
!1 = !{null, !"align", i32 8}
!2 = !{null, !"align", i32 8, !"align", i32 65544, !"align", i32 131080}
!3 = !{null, !"align", i32 16}
!4 = !{null, !"align", i32 16, !"align", i32 65552, !"align", i32 131088}
!5 = distinct !{!5, !6}

Thanks @jdoerfert! I was going to implement a fix for that, but have not managed to do it yet. See a couple of inline comments below.

In D120129#3408168, @jdoerfert wrote:

You don't need to run it. If you use this command line it doesn't crash?

In D120129#3408088, @jdoerfert wrote:

Godbold seems to not have included this:

$ cat test.cpp
int main(){return 0;}
$ clang++ -fopenmp -fopenmp-targets=nvptx64 -Xopenmp-target -march=sm_80 test.cpp
... static bool llvm::isa_impl_cl<llvm::ConstantAsMetadata, const llvm::Metadata *>::doit(const From *) [To = llvm::ConstantAsMetadata, From = const llvm::Metadata *]: Assertion `Val && "isa<> used on a null pointer"' failed.

Accidentally placed this revision ID in commit message related to other patch. Reverted that.

@tra Added appropriate tests for kernels. Looks like that kernel functions get external linkage regardless static specifier, so their parameters alignment is kept untouched.
To ensure that it's true, altered clang/test/CodeGenCUDA/device-fun-linkage.cu and added appropriate assertion in getFunctionParamOptimizedAlign - it checks module metadata and identifies if the function is marked as kernel or not.

@dexonsmith Previously you requested changes to this revision. Please let me know if you still see some problems - updated the revision, please see my last comments for details. Thanks!

@tra Thanks for your comments! Updated the patch according the discussion about forcing alignment 16.

Fix formatting according to pre-merge checks. @dexonsmith Please let me know if you have some comments on the changes. I added more info in my previous comment. Thanks!

Fix formatting problems found on pre-merge checks

Keep private headers untouched

Fix typo

@dexonsmith Thanks for your comments! Decided to choose the last approach that you mentioned - keep and use PersistentId regardless build type or any macro. The problem with #if LLVM_ENABLE_ABI_BREAKING_CHECKS is testing with LLVM_ABI_BREAKING_CHECKS set to FORCE_OFF during initial CMake configuration - some tests rely on PersistentId. I suppose that having PersistentId present in all builds is definitely worth small CPU overhead compared to the current state.

In D120714#3353070, @craig.topper wrote:

Here is a previous thread on this topic https://lists.llvm.org/pipermail/llvm-dev/2016-July/101842.html

In D120129#3338254, @tra wrote:

FYI. I've recently proposed to pass the values directly, instead of a byval pointer: https://discourse.llvm.org/t/nvptx-calling-convention-for-aggregate-arguments-passed-by-value/5881
It needs additional changes to get lowering done correctly. I should have them ready in about a week.

@tra Thanks for your review! Fixed tests.

@spatel Fixed the issue we were talking about - could you give a review on the updated patch please?

In D118769#3290443, @spatel wrote:

For the files that still run multiple passes, I think we're trying to show that the combination of passes are cooperating to produce the expected/optimal result.
Can we move those to "llvm/test/Transforms/PhaseOrdering" and run "-O2" instead? That would show that we have the expected result from the typical optimization pipeline without needing to name specific passes. Then if something alters the pipeline and breaks the expected result, we'd know sooner.

In D117302#3248293, @spatel wrote:

I already drafted a patch for it while looking at this, so I can clean that up and post it (the harder part is adding a pile of tests to check FMF propagation!).