Thanks!

ping?

ping?

ping?

LGTM apart from a bunch of formatting issues. I haven't marked all of them, please just run clang-format or clang-format-diff.

In D60772#1470073, @arsenm wrote:

I don't understand the problem being solved here. Who/what is this intended to benefit? An out of bounds access is going to be undefined.

LGTM

LGTM

Thanks! This looks good, except I still believe the test case can be simplified.

LGTM

Why does this return false? A 1-bit immediate is either 0 or -1, both of which can be represented as inline constants everywhere.

Address review comments.

In D61494#1489467, @arsenm wrote:

Is it still possible to make use constant addresses in some cases? If there are no calls for example

• Use '\n' instead of "\n"
• Cleanup and extend tests

Add a test/CodeGen/MIR/AMDGPU test.

In D61491#1489425, @arsenm wrote:

But these are often -1?

After some more investigation, you're right.

In D61313#1493137, @hakzsam wrote:

In D61313#1489140, @nhaehnle wrote:

That's an interesting catch. Basically a Write-after-Write hazard.

Though this seems like the wrong place to add this check, since the function is specifically called canMoveInstsAcrossMemOp.

I rather suspect that instead addToListsIfDependent needs to be fixed instead, so that when the scan hits the S_CMP_EQ_U32 it recognizes the WAW hazard and adds it to the instructions to be moved.

I also wonder if this is related to D60459.

How do I recognize the WAW when scanning S_CMP_EQ_U32? I'm not sure to follow you.

Added a minimal verifier-only test before commit.

I've changed the patch to not report the location in the non-inline case on the basis that it's an internal compiler error. Will keep it up for a while here and commit if there are no further comments.

Changing the error reporting not to report the instruction location
in the non-inline asm case.

In D61489#1490079, @efriedma wrote:

Given that, there needs to be some way for error reporting to distinguish the two errors. When we run out of registers due to a non-inline-asm construct in clang, we want to make it clear to the user that the issue is a compiler bug, not user error. This means we need to trigger the code in the clang driver that asks for a bug report and generates preprocessed source.

Of course, that isn't fundamentally tied to calling abort() from the register allocator; some alternate mechanism for reporting ICEs which doesn't immediately kill the process would be reasonable.

In D61525#1490187, @rampitec wrote:

In D61525#1490186, @arsenm wrote:

What does the signedness of the multiply even mean?

It does not. There is problem with the instruction itself.

I wonder if this is related to D61313?

That's an interesting catch. Basically a Write-after-Write hazard.

Committed. I believe the procedure for SVN access is still that you send Chris Lattner an email to ask about it.

LGTM, with the understanding that the test case which we have in our internal branches will make it upstream.

I think there are some misunderstandings here. None of the IR passes require LCSSA. The problem is in getting the divergence data into the SelectionDAG.

LGTM

Mesa uses +DumpCode as well. Do we have a good solution for generation both ELF and textual output?

Oh wow. That's an important fix indeed :) LGTM

Okay, you've convinced me. I only hope we can move forward with GlobalISel and do it right there.

From the point of view of the design of all these interface, It's too bad we can't fix this in post. From an overall standpoint, it's actually better to get the register classes from the beginning, so sure, let's go with this kind of approach.

LGTM, whether you do the NoAlias between constant 32-bit and buffer address space here or separately. (Making NoAlias between constant 32-bit and the other existing address spaces should definitely be a different patch.)

LGTM

Hmm, this is fragile -- I think your reasoning about domination is mostly sound, except when there's uniform control flow inside the loop itself, in which case I'm not sure. That said, it seems to me that the domination check is conservative, and your change shouldn't break anything that wasn't broken before, and a proper fix is potentially much more difficult.

Awesome :)
LGTM

FWIW, the problem is a bit more involved than that. Consider

bool value = ...;
if (divergent condition) {
  use(value);
}

So the undefinedness of inactive bits in this case is not due to suboptimal lowering of NOT, but inherently due to the fact that we're in a different region of control flow.

Hmm, this is one of those cases where it'd be awesome to have a Godbolt for the tests.

+1 for better error messages! LGTM.

In D57825#1390608, @arsenm wrote:

In D57825#1390606, @nhaehnle wrote:

In D57825#1390457, @arsenm wrote:

@nhaehnle can you look at the InstCombineSimplifyDemanded change? I was a bit confused by the assert you added

The background of this is that if TFE/LWE is enabled, the SimplifyDemanded logic won't work as-is; but it should also never be hit, because intrinsic calls with TFE/LWE should have a struct return type (e.g. {v4f32,i32}) and the SimplifyDemanded logic doesn't support looking through that. The assert double-checks that.

In hindsight, it would be possible for somebody to manually create malformed IR which calls image intrinsics with TFE/LWE enabled but with a vector return type. That would be an error, and one could argue that the code should produce an error instead of an assert. It would require a broken frontend or manually written IR, though.

I would expect TFE to be turned on by the usage of the struct type. We should probably add a custom verifier check for this

In D57825#1390457, @arsenm wrote:

@nhaehnle can you look at the InstCombineSimplifyDemanded change? I was a bit confused by the assert you added

Looks reasonable to me. Maybe give a little more time for people to give feedback? Though this has already been on llvm-dev without opposition...

Why can't we recognize this as a pattern? Basically, it's just (src & (1 << thread_idx)), and thread_idx can be matched as a sequence of mbcnt intrinsics.

Did you actually test this? The shift-by-3 should be unnecessary.

LGTM

It looks like this was never committed. What's the next step here?

I still find the code as-is a bit dubious, but we no longer need this change and the review process tends to be a bit of a pain, so I'm dropping this.

LGTM

In D57681#1383030, @arsenm wrote:

Does it actually matter? I thought since this needs to be a constant, this just needs to not crash

Hi Valery, I really like the way the different cases are listed in the explanatory comment at the top of the file, and I believe those cases are correct. Would it be possible to restructure the code in a way that follows those cases? I think that would make it much easier to follow.

LGTM

The only user of canReadVGPR is addUsersToMoveToVALUWorklist. Since the intended semantics of canReadVGPR aren't at all clear from the name, might I suggesting folding it into its only user?

Thanks, LGTM

LGTM

LGTM

In D55369#1321530, @arsenm wrote:

I thought mesa was moving to stop using the relocations at all for this?

Please make the change apply to Mesa3D as well.