ping

In D52221#1240735, @sameerds wrote:

I changed the first line in the change description to make it explicit that this is a workaround for DA, but it is not showing up here. I just used "arc diff master" ... how do I get the web interface to show the updated description?

Yes, thank you, this is clearer. I'm fine with the change, but please do address Matt's comments.

Good to hear the new DA can handle this. We need lowered switches for the control flow lowering anyway, though, so we may as well do it this way.

Okay, thanks for the explanation, that seems fair.

Adding @tpr as a subscriber due to the (admittedly maybe a bit indirect) MsgPack connection.

Thanks, this mostly looks good to me. Looks like this may be running into a serious limitation of the ISel infrastructure with commutativity / associativity, but it makes sense to land this patch without addressing it. I do have one last question.

In D52018#1235002, @critson wrote:

In D52018#1232860, @nhaehnle wrote:

Why the detour via V_CVT_F16_F32 instead of selecting an fp16 1.0 constant directly?

It is not possible select an fp16 constant with V_CNDMASK_B32.
In principle the VOP2 version could be used to select an inline literal, but only when the i1 is vcc.

This still needs to be adjusted for D52087, and I have one minor comment, but mostly LGTM.

LGTM

• Cleaned up the pattern condition to take inline constants into account
• Add tests for inline constants and uniform values ending up in VGPRs
• update_llc-ify the tests

LGTM

What about sitofp?

In D51993#1232208, @RKSimon wrote:

Any perf numbers? For instance, on x86 -gen-dag-isel

As for the testcases, what about vectorized multiplicaton, i.e.:

%vec1 = load <8 x i4>, ...
vec2 = load <8 x i4>, ...
%ext1 = zext <8 x i4> %vec1 to <8 x i32>
%ext2 = zext <8 x i4> %vec2 to <8 x i32>
%mul = mul nuw nsw <8 x i32> %ext1, %ext2
... then extractelement and add up the result
... or possibly the same thing without the zext

The TableGen itself looks good to me, except for one nitpick (inline).

Thanks! Some small issues let, and please also add a test case for the "simplifyDemanded" implementation.

One nitpick, apart from that LGTM.

LGTM

LGTM

This patch should already add the tests for the new analysis, shouldn't it?

Thanks, the TableGen part looks good to me. I can't speak for the actual RISC-V changes.

In D51203#1216951, @mareko wrote:

In D51203#1214382, @nhaehnle wrote:

Looking at SelectionDAGBuilder::visitGetElementPtr, nuw is set under certain conditions for inbounds getelementptr. I suspect we should be able to make most GEPs inbounds in Mesa - it just means that we never, not even temporarily, try to take addresses outside of properly allocated memory objects (buffers, arrays of descriptors).

Would the combination of:

• check NUW here
• create inbounds GEP make good use of SMEM/SMRD immediates?

The code in SelectionDAGBuilder::visitGetElementPtr is unsafe for our case, because it doesn't know that the addition of 32-bit addresses is performed in 64 bits. Even x+4 can overflow in 32 bits but not 64 bits. The GEP can be "inbounds", but it doesn't change anything. However, we can hackishly use the inbounds flag to mean that the addition is safe, because GEP with inbounds and offset <= INT_MAX is converted to "add nuw".

It is interesting to note though that clang-format changed so much in this file.

Guard sanity check with EXPENSIVE_CHECKS

Hi Simon, can you please confirm that this is still the latest version of this change? There were some edits to D50433 since the last comment here...

Thanks for the changes.

I'm not very familiar with this code either, but the change looks reasonable to me.

That sounds like the way to go is testing just the NUW bit here in SelectSMRDOffset?

That would be good, yeah. Note here it's not so much a case of preserving NUW as it is of deducing as much NUW as possible from getelementptrs.

Looking at SelectionDAGBuilder::visitGetElementPtr, nuw is set under certain conditions for inbounds getelementptr. I suspect we should be able to make most GEPs inbounds in Mesa - it just means that we never, not even temporarily, try to take addresses outside of properly allocated memory objects (buffers, arrays of descriptors).

Right, that matches my understanding: the SMRD/SMEM instruction does a 64-bit addition, so if the 32-bit (add X, imm) were to have an unsigned wraparound, moving it into the immediate of the SMRD/SMEM would remove the wraparound and therefore be incorrect.

Nice catch. One suggestion for improvement, apart from that LGTM.

Thanks. I don't see a test that actually sets glc, please add one before committing.

LGTM

In D50433#1212161, @simoll wrote:

In D50433#1210654, @nhaehnle wrote:

Also, maybe I missed it, but could you state clearly in a comment what the assumed preconditions are for correctness?

I will add comments to the class declarations of the DA and SDA - they require the CFG to be reducible.

The operand name / encoding bits in TableGen should really be changed as well to indicate the overload between R128 and A16.

Please do a style pass to capitalize all variable names according to the LLVM coding style.

I find it a bit concerning that we seem to have different semantics for these instructions depending on the environment.

In D50629#1209494, @arsenm wrote:

In D50629#1209249, @nhaehnle wrote:

Always do the sanity check in debug builds.

I was originally worried about how expensive this is, but it
doesn't show up in the running time of all the tests in
test/CodeGen/AMDGPU.

These are by design almost all small, so that makes sense. It might not be representative in the real world

Always do the sanity check in debug builds.

LGTM

Some smaller nitpicks, but mostly looks good to me!

LGTM

Thanks! LGTM

Agreed that LCSSA doesn't help either.

LGTM

LGTM

Great, thanks!

LGTM

LGTM

LGTM

Thanks for doing this.

As far as I can tell, there are four kinds of operands when it comes to fixed-length encoding / decoding:

What's the status of this patch?

This makes sense to me. One nit-pick, apart from that LGTM.

I don't think doing this as an IR pass has any advantage, so this is fine.

LGTM

Factor out SIInstrInfo::hasUnwantedEffectsWhenEXECEmpty

In D48431#1139259, @arsenm wrote:

We really need to invert how this pass works

Rebased.

Rebased.

Rebased.

Rebased.

Preserve the case in preprocessor guards for GenericEnum and GenericTable.

Ping. Does this look alright?

I've had some time to let this sink in now.