Ping to @fhahn!

Other approach worth looking at is to push args in Clang.cpp. We can look at AddAArch64TargetArgs() and pass default CPU if -mcpu or mtune is not provided.

In D41766#4474557, @fhahn wrote:

I have a high-level question for this patch. I understand that the verify function aims to verify patterns to be present in certain order but insertion is not guaranteed to be in the same order.

Does it make sense to have this verification in the first place?

The machine combiner applies the pattern in order, so the goal is to ensure that the most profitable patterns are processed first. Did you encounter cases where it wasn't possible to fix the insertion order?

Hi @fhahn
We are testing AArch64 builds with EXPENSIVE_CHECK=ON and SPEC 2017 and llvm test-suite are failing with the assert in this patch.

I see the problem of calculating incorrect register usage. However, is it just limited to loop invariants?

LGTM

A couple of nits

Looks fine

Hi Ashutosh,
Did you do benchmarking with this patch? For example, does this benefit/regress SPEC'17 by any means on x86 or AArch64 platforms? I see that it is disabled by default but it is useful to understand the impact. Thanks!

Can we please more verbose title for the patch?

Can we please have a bit more detailed description for this patch? What problem this patch is trying to solve?

rebase + address comments

If you want to you can emit some debugging message after clearing the map and have a llc lit test with the debug flag to CHECK the debugging message.

Please remove NFC due to the new context. @JonChesterfield found an issue with recursions in the base patch. The following case depicts the scenario

Commit 660cae84c3144a42272daa16415fc9a2532773c4 reverts the base patch due to an unsupported scenario but thankfully this patch fixes that issue. Please rebase and keep the support intact.

optimize conditions

Ping to @arsenm

address review comments

address review comments + rebase

Ping to @arsenm and @rampitec!

add comment about keeping indirect calls handling separate

Fixed one more test after rebase.

use assert

In D103138#2787055, @foad wrote:

You cannot recover after an "unreachable" or a failed assertion.

C [...] returns gracefully returns in release builds.

No it doesn't. In a release build llvm_unreachable will usually expand to __builtin_unreachable() or __assume(false) and the optimising compiler will remove any code after that point because it is allowed to assume that it is not reachable.

use NSDMI for ModuleCG

address review comments

LGTM

In D101847#2755717, @arsenm wrote:

I think at this point I'd prefer to just not worry about the cast stripping case and go back to just handling the direct callee.

This pass should not be required for correctness, and is only because opencl is still not linking the device libraries correctly such that the linked in functions have the correct subtarget/subtarget features. Long term I would like to move this pass into a form that does conservative merges of attributes, which can safely give up on anything indirect looking

In D102177#2750256, @sebastian-ne wrote:

In D102177#2750094, @madhur13490 wrote:

In D102177#2748278, @arsenm wrote:

We have another proposal we were working on to rearrange these a bit differently. We need to account for a few more inputs in the layout

As long as this remains in GFX land, we should be fine with it because our new proposal is for compute only (as of now).

I would like to keep the same calling convention in compute and graphics. At least regarding the stack pointer and others, because I don’t see a compelling reason to diverge even more. Actually, I’d like it if they were more common than they are now, because we implement some things twice at the moment.
The compute proposal should work just fine; if we move the stack and frame pointer, we end up with the same benefits as in this patch. I commented on the internal proposal for this (I hope I found the right one?).

In D102177#2748278, @arsenm wrote:

We have another proposal we were working on to rearrange these a bit differently. We need to account for a few more inputs in the layout

I feel we should probably discuss more about the optimal solution than the current one. I have a gut feeling that the problem pass is trying to solve is a flavor of graph coloring problem where we have to color the interference graph with K number of colors where K is the number of register banks. We may need to redo the interference graph in this pass and then attempt the coloring.

remove new test and merge in existing one

keep tests alone as discussed offline and add explicit subtarget to test

In D96179#2693680, @rampitec wrote:

In D96179#2693666, @madhur13490 wrote:

I hope this doesn't run into the issues I observed in D98884. Doing bootstrap builds locally would strengthen testing.

That's the point. It is effectively default on for a long time already.

I hope this doesn't run into the issues I observed in D98884. Doing bootstrap builds locally would strengthen testing.

change tests at HEAD

In D98884#2681986, @jdoerfert wrote:

Are we sure this is not a single bad cast that evolves into something different if assertions are disabled. Is the cast problem located, that should be easy after all given the minimal reproducer in PR49861?

In D98884#2680577, @arsenm wrote:

In D98884#2678593, @madhur13490 wrote:

This change has wider impact than was expected as it affects several components and backends. Bootstrap builds fail with this change and as the buildbots show, this change generates a buggy Tablegen binary in release mode.

Debugging:

1. Disabling the newly added code in hasAddressTaken(), we get sane Tablegen and builds pass. To note, debug builds pass with the change. This clearly shows that codegen is functionally different in release and debug mode for Tablegen.
2. Doing “ninja check-all” on stage1 build does not help although we have ~90000 tests.
3. Running valgrind also did not reveal any new things.
4. Doing ASAN build of the compiler led to several errors and drawing any conclusion from it is close to impossible for this patch.

However, if I disable IPSCCP pass on top of this patch then everything turns green. I tried to fix trivial issues in the pass but it seems the pass needs deeper and major fixes to make this patch land.

Till we fix issues in IPSCCP, this patch stands reverted. The whole intention of this patch was to handle a subset of cases for AMDGPU backend, specifically, needed for https://reviews.llvm.org/D99347.

This doesn't necessarily mean there's a problem with IPSCCP. What is the failing IR?

This change has wider impact than was expected as it affects several components and backends. Bootstrap builds fail with this change and as the buildbots show, this change generates a buggy Tablegen binary in release mode.