I guess this particular case could be handled by improving the v_perm matching in SITargetLowering::performOrCombine, e.g. add a cases for or (op x, c1), c2 -> perm x, x, permute_mask(c1, c2) and or (perm x, x, c1), (op y, c2) -> perm x, y, permute_mask(c1, c2).

Overall this seems reasonable. The only alternative I can think of would need more complicated isel patterns that do the reassociation gated by predicates that check the divergence, something like:

let Predicate = doesNotHaveXNOR in
def : GCNPat<
  (i32 (xor (xor_oneuse i32:$src0, i32:$src1), i32:$src2)),
  (i32 (V_XOR_B32 $src0, (V_XOR_B32 $src1, $src2))),
  [{ return src0->isDivergent() && !src1->isDivergent() && !src2->isDivergent(); }]
>;

... and lots of commuted versions of the same thing, and the same for any other isel pattern that matches something that could be reassoicated. So that doesn't sound very scalable.

LGTM, thanks!

LGTM. It would be nice to improve the codegen as noted inline but I'm not sure how to implement that.

LGTM, thanks!

or have the utils function return a vector of APInt

Previously we were falling back to the DAG on selection
failure, where it would emit this error and then fail again.

Make v8i16/v8f16 legal

In D117544#3250636, @piotr wrote:

In our usual compile-time tests it shows 0.056% degradation on average, worst case 0.9%.

Did you notice any compile-time degradation from your fix?

As a bit of background, the OldOutOfOrder test was introduced by @nhaehnle in a large refactoring in D54231. I think it's just a performance optimisation: if the old state had events that completed out of order, then any use of the corresponding registers would have to be preceded by a "waitcnt 0", so there's no need to reprocess that block because the waitcnts are already as strict as they can be. But this goes wrong in the case where the merge introduces a wait on a particular register that had no wait in the old state (so no waitcnt would have been generated for it the last time the block was processed).

LGTM, thanks!

I don't understand. Why do we need to handle llvm.r600.* intrinsics on amdgcn subtargets? Why does globalisel need to handle them, when it doesn't support r600?

LGTM. I always assumed it was intentional that legalization rules had to be ordered in just the right way.

It looks like for some tests you manually optimized the IR, but for other tests you updated the expected ISA. Any reason for the difference?

In case anyone needs to make equivalent changes downstream, this patch was created with: sed -i 's/{{\\\[}}/[/g;s/{{\\\]}}/]/g' $(grep -lr '{{\\[][]}}' test)

It seems like people are mostly happy with the design now, so I am being a bit more picky with my review comments!

LGTM, thanks!

Looks obviously fine, thanks!

Add a FileCheck test.

In D116469#3235782, @craig.topper wrote:

@foad were you able to come up with a test for this?

In D116270#3234484, @alex-t wrote:

Removing this guard leads to the infinite transforming the pattern back and forth in SITargetLowering::reassociateScalarOps and DAGCombiner::ReassociateOps().
The former transform (xor (xor uniform, divergent), -1) to (xor (xor uniform, -1), divergent) but the latter one transform it back by applying this rule:

if (N0.hasOneUse()) {
 ** // Reassociate: (op (op x, c1), y) -> (op (op x, y), c1)
  //              iff (op x, c1) has one use**
  if (SDValue OpNode = DAG.getNode(Opc, SDLoc(N0), VT, N00, N1))
    return DAG.getNode(Opc, DL, VT, OpNode, N01);
  return SDValue();
}

Alternatively we could pass DstOps to the constant fold functions.

In D116270#3231657, @alex-t wrote:

Once again, in my case BOTH nodes (not,xor) are divergent!

 %s.load = load i32, i32 addrspace(4)* %s.kernarg.offset.cast, align 4, !invariant.load !0
DIVERGENT:       %v = call i32 @llvm.amdgcn.workitem.id.x(), !range !1
DIVERGENT:       %xor = xor i32 %v, %s.load
DIVERGENT:       %d = xor i32 %xor, -1
DIVERGENT:       store i32 %d, i32 addrspace(1)* %out.load, align 4

In D116270#3231609, @alex-t wrote:

Now:

We select the divergent NOT to V_NOT_B32_e32 and divergent XOR to V_XOR_B32_e64. The selection is correct but we missed the opportunity to exploit the fact that even divergent NOT may be selected to S_NOT_B32 w/o the correctness lost.

+ @pcc for the partition stuff. But it all LGTM.

In D116270#3209307, @alex-t wrote:

This looks like a regression in xnor.ll :

	s_not_b32 s0, s0                        	v_not_b32_e32 v0, v0
	v_xor_b32_e32 v0, s0, v0                        v_xor_b32_e32 v0, s4, v0

but it is not really. All the nodes in the example are divergent and the divergent ( xor, x -1) is selected to V_NOT_B32 as of https://reviews.llvm.org/D115884 has been committed.
S_NOT_B32 appears at the left because of the custom optimization that converts S_XNOR_B32 back to NOT (XOR) for the targets which have no V_XNOR. This optimization relies on the fact that if the NOT operand is SGPR and V_XOR_B32_e32 can accept SGPR as a first source operand.
I am not sure if it is always safe. The VALU instructions execution is controlled by the EXEC mask but SALU is not.

SITargetLowering::reassociateScalarOps exists to fix the instruction selection that is done in a wrong way.

In D116807#3227570, @arsenm wrote:

Legality rules can't be context dependent on the specific operands used, but IIRC here it only wants to use it if the operands are constants

TBH I don't understand why D99283 introduced this target lowering hook in the first place. Comments on the review say "The annoying part (for AArch64) is that the legality checks don't work with custom legalization". Did isLegalOrCustom not exist back then? Or does it not do the right thing? (I don't see any tests failing with this patch.)

Further cleanups are possible:

1. Change CombinerHelper::matchBitfieldExtractFromShrAnd to use getPreferredShiftAmountTy for the shift-amount-like operands of G_UBFX, like all the other G_*BFX combines do. Change AArch64's G_*BFX legality rules to match.
2. Remove isConstantUnsignedBitfieldExtractLegal since it doesn't seem to do any more than a standard legality check.

In D115269#3226623, @dyung wrote:

The test added in this change fails on build bots that do not build the s390x target. Can you please take a look?

https://lab.llvm.org/buildbot/#/builders/139/builds/15573

******************** TEST 'LLVM :: MC/GOFF/ppa1.ll' FAILED ********************
Script:
--
: 'RUN: at line 1';   /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/bin/llc -mtriple s390x-ibm-zos < /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/llvm-project/llvm/test/MC/GOFF/ppa1.ll | /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/bin/FileCheck /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/llvm-project/llvm/test/MC/GOFF/ppa1.ll
--
Exit Code: 2
Command Output (stderr):
--
/home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/bin/llc: error: : error: unable to get target for 's390x-ibm-zos', see --version and --triple.
FileCheck error: '<stdin>' is empty.
FileCheck command line:  /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/build/bin/FileCheck /home/buildbot/buildbot-root/llvm-clang-x86_64-sie-ubuntu-fast/llvm-project/llvm/test/MC/GOFF/ppa1.ll
--

SIInstrInfo::lowerScalarXnor() is dead after your patch

LGTM.

I don't really have an opinion on the patch, but I'm curious.

In D116616#3222181, @spatel wrote:

In D116616#3221740, @foad wrote:

It seems like you're using knownbits information to derive range information. It would be good to do this more universally, and in both directions.

Yes - we do have a combination analysis called "computeConstantRangeIncludingKnownBits", so that should be more powerful.
But that is currently a static helper function in ValueTracking, so it would have to be made visible, and it would be good to have more tests to show where the extra logic gives us a better result. Ok to make that a TODO item?

It seems like you're using knownbits information to derive range information. It would be good to do this more universally, and in both directions.

In D116469#3220809, @craig.topper wrote:

In D116469#3219140, @foad wrote:

This patch seems to change codegen in test/CodeGen/AMDGPU/lshl64-to-32.ll but I haven't managed to understand whether the change is good, bad or indifferent.

I'm not seeing any failure for that test in my local runs. The test appears to be generated by update_llc_test_checks.py and rerunning the script produced no changes. Am I missing something?

This patch seems to change codegen in test/CodeGen/AMDGPU/lshl64-to-32.ll but I haven't managed to understand whether the change is good, bad or indifferent.

Rename APInt::getMinSignedBits->getSignificantBits

Both changes look good to me but it's not clear how they are related, and it seems odd to include the generic change in a patch which claims to be AMDGPU-specific. Would it makes sense to commit the generic change separately first?

What was the effect of inserting multiple branch instructions? Did it fail MIR verification?

I like it.