Updated commit message: the highest value that can be represented with
immediate + inline is 4156 = 4092 + 64.

In D37205#855583, @arsenm wrote:

LGTM.

How about in unittests/ADT/TripleTest.cpp?

TripleTest.cpp has some minimally meaningful tests though. So a test of amdgcn-amd-amdpal should be added there. Apart from that the patch looks good.

LGTM

Some comments on what I noticed. Probably best if somebody else has a look as well.

One comment, apart from that LGTM.

LGTM

One remaining style nitpick. LGTM with that fixed.

Thanks. Assuming we agree on the derivative calculations logic I added in a comment, this LGTM.

Looks mostly good, but I do have a couple of comments.

One question, apart from that looks good.

One minor comment. Either way, LGTM.

Some minor comments. In addition, I think it can be simplified, and we probably want the intrinsic to be convergent, because sinking WQM computations into a non-uniform branch could mean that the computation becomes non-WQM for practical purposes.

This was submitted long time ago.

So, one thing that's not clear to me with is the semantics of how the update.dpp intrinsic is supposed to enable WQM or WWM. In your sequence of instructions, if you just put a WQM/WWM flag on the update.dpp intrinsic, how does LLVM know whether the regular ALU intrinsics in between should run in WQM/WWM or not?

Low & high bits and use cases aside, isn't this something that could be covered by adding support for PC in the read_register intrinsic?

In D31762#735590, @mareko wrote:

In D31762#735585, @nhaehnle wrote:

Is it at all possible to get merged shaders where either part has thread_count == 0? We might want a way to annotate branches so that the skip-jump for EXEC=0 is not introduced.

Yes, thread_count == 0 is possible, and it's explained here: https://patchwork.freedesktop.org/patch/152356/

In D31762#735528, @mareko wrote:

In D31762#735505, @nhaehnle wrote:

I don't see anything wrong with the code.

I agree that the design is a bit iffy. It's almost like these intrinsics are something that is part of the calling convention. But even these intrinsics cannot quite lead to optimal code for merged monolithic shaders, because there's an unnecessary initialization of EXEC in the first part of the shader.

Since what we need to do here in general really doesn't fit well into LLVM IR semantics, I suspect that no matter what we come up with, it's bound to be ugly. So we might as well go with this particular solution here.

Merged monolithic shaders set exec = -1 and then they use "if (tid < thread_count) ...". I think that's the only way to jump over the conditional code right now if the wave has thread_count == 0. If we don't want v_mbcnt+v_cmp, we could do something like "if (amdgcn.set.thread_count(n)) ..." that sets EXEC regardless of current EXEC and skips the conditional for thread_count == 0. The performance of that solution is unlikely to justify the implementation effort.

Address review comments and apply some more formatting fixes.

Address review comments.

I don't see anything wrong with the code.

LGTM

There might be a benefit to choosing a larger alignment, like 64 bytes, due to cache line alignment.

LGTM

When applied to current trunk, this code can get into an infinite loop. Please see the sample LLVM IR at https://paste.debian.net/926533/

In D28993#662995, @mareko wrote:

In D28993#662978, @nhaehnle wrote:

I see the same problem as Tom here. Do those shaders use read-only SSBOs? If so, this could perhaps be done at the Mesa level. But even then, there'd be a problem if the same memory is bound to two different SSBOs, and one of them is written to, unless the SSBO is marked 'restrict'.

Can you be more specific about why it's incorrect? I only see an issue with L1 coherency (GLC=0).

I haven't looked in too much detail yet. I assume getelementptr doesn't work with these pointers, so it would be good to have a negative test which ensures that GEP use fails.

I see the same problem as Tom here. Do those shaders use read-only SSBOs? If so, this could perhaps be done at the Mesa level. But even then, there'd be a problem if the same memory is bound to two different SSBOs, and one of them is written to, unless the SSBO is marked 'restrict'.

Backing out the effectively dead FMF code, but leave the corresponding
FIXMEs in place.

In D28675#659120, @hfinkel wrote:

In D28675#659097, @nhaehnle wrote:

Yeah, I personally feel more comfortable having the exact same change on the branch.

I meant that having getFlags() always return something dereferenceable seems safer.

Yeah, I personally feel more comfortable having the exact same change on the branch.

Drop unnecessary getNode()s.

But you can't because they'll crash [...]

@arsenm, unfortunately I can't add flag-based tests, since as @hfinkel points out, you can't actually get the flag on an FMA or FMAD in the DAG. @hfinkel, the idea was to put the check in already anyway, since by the discussion with @spatel, the plan is to add FMF for all nodes eventually. That seems reasonable to me.

Formatting of FIXMEs

LGTM.

Added the DefaultFlags. It does fix @spatel's example for me. Anything else?

Obviously it'd be even easier to skip the getFlags() thing entirely now, since it can never evaluate to true. But the right way forward is to extend support for FMF, so...

In D28675#650423, @spatel wrote:

The patch will cause a crash [...]

Add FIXMEs. @hfinkel, do I understand your comment correctly that this patch is
good to go?

1. Take individual UnsafeAlgebra flags into account.

Thanks! We generally use CHECK-LABEL only for the start of a function, not for labels inside a function. The idea is that the label helps FileCheck get less confused when there are many sub-tests in a single test file.

The approach is unfortunately not correct in general as far as I can tell. Say you have

LGTM

LGTM

I don't see the change that merges the FP_EXTEND / FP_ROUND cases. But the end result LGTM.

LGTM

LGTM

This looks like it's missing a rebase onto the rest of the series.

LGTM

LGTM

LGTM

A test case would be good. Apart from that, LGTM.

One minor comment, LGTM otherwise.

LGTM

LGTM

LGTM

LGTM

LGTM

LGTM

Thank you for taking a look!

In D26348#617266, @mehdi_amini wrote:

We need to clarify, there are two parts in my comments:

1. "divergence" is not common in LLVM. I stand on this point: the DivergenceAnalysis is a GPU specific pass and is hardly part of the core semantic of the IR.

LGTM

Thank you for taking a look.

• Use an enum instead of OnlyLegalOrCustom
• Handle a corner case where shift amounts are too large for the native shift type

• Test case using alloca
• Unify getting the MUBUF instruction offset

Obsoleted by changes to D27346.

Don't split the files, just double the RUN lines in the existing files.

In D27346#611748, @RKSimon wrote:

Why is this and D27347 better than just tweaking the check-prefixes?

Ping.

Rearrange the logic. It looks quite readable to me this way, and
clang-format-diff agrees with the formatting.

Rebased on D27260.