Yes, this makes sense to me as well. Patch LGTM.

In D81537#2207249, @qiucf wrote:

In D81537#2193216, @uweigand wrote:

This doesn't look correct. As far as I can see, none of the conversion functions were actually changed to handle strict operations. For one, you'll need strict variants of all the PowerPC-specific conversion operations, use them in all the conversion subroutines, and consistently track their chain nodes.

The patch only adds a strict variant of the direct move, which seems to me the only operation where actually a strict version is not required ...

Thanks for pointing them out! I have something unclear about chains:

(1) If a constrained operation is expanded into several FP nodes a-b-c, they should all have chain set to former operation (b's chain is a, c's chain is b) even if they have def relationship?

This doesn't look correct. As far as I can see, none of the conversion functions were actually changed to handle strict operations. For one, you'll need strict variants of all the PowerPC-specific conversion operations, use them in all the conversion subroutines, and consistently track their chain nodes.

This LGTM now.

I'm not sure what specific problem you're trying to solve here, but this change in itself looks incorrect.

Sure, of course assuming the test still passes.

LGTM for SystemZ as well.

LGTM with the format fixes.

This LGTM now.

See inline comments about chain handling.

The one issue I still see is that when falling back to library calls on older platforms, signaling and non-signaling compares both are treated the same. But that's really a pre-existing problem because this is not fully supported by libgcc (see the comment in TargetLowering::softenSetCCOperands), so it's not really a problem with this patch.

In D80833#2123508, @aganea wrote:

In D80833#2109172, @uweigand wrote:

Hmm, with clang-cl it seems the driver is trying to use this:
Target: s390x-pc-windows-msvc
which of course doesn't exist. Not sure what is supposed to be happening here, but it seems that it's falling back on s390x-linux since on s390x, Linux is currently the only supported OS.

I'm seeing some of the tests are setting the target explicitly %clang_cl --target=x86_64-windows-msvc. Would that work on your machine? Or should I do UNSUPPORTED: s390x ?

Committed as 4c5a93bd58bad70e91ac525b0e020bd5119a321a.

Drop AllowNoUniqueAddress parameter; address review comment.

In D81583#2138127, @qiucf wrote:

Thanks for this patch!

If I understand correctly, only isEmptyRecord/isEmptyField and places checking any field is zero bit-width may need change for this? Since isSingleElementStruct and isHomogeneousAggregate just use them to skip empty fields in aggregate. I didn't see direct checking for empty fields on PowerPC. So all change needed on PPC seems to be generic. By enabling AllowNoUniqueAddr in these methods, case in https://lists.llvm.org/pipermail/llvm-dev/2020-July/143141.html can be correctly recognized as homogeneous aggregate.

In D81583#2137277, @efriedma wrote:

I'm tempted to say this is a bugfix for the implementation of no_unique_address, and just fix it globally for all ABIs. We're never going to get anything done here if we require a separate patch for each ABI variant clang supports.

Handle array of empty records correctly.

Yes, this does look like an obvious typo. LGTM!

Rebased against mainline.

This was actually fixed in a different way here: https://reviews.llvm.org/D75014
I'm closing this review now.

Another ping ...

A couple of cosmetic changes inline, otherwise the LGTM. Thanks!

In D82622#2122708, @Higuoxing wrote:

In D82622#2122562, @uweigand wrote:

This commit causes test case failures on s390x (unfortunately hidden as the build bot was already red due to an unrelated issue):
http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/33624

http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/33624/steps/ninja%20check%201/logs/FAIL%3A%20LLVM%3A%3ADWARF-debug_info.yaml

At first glance, seems like this could be endian-related?

The failed tests are removed in 8032727a43ca678b0b923abaa04638f500a060d6. Thanks.

I've now updated the knownbits.ll test case to make it less fragile and still test what it is supposed to test (commit e9c6b63). The problem was use of "undef" (which makes it susceptible to collapse as generic optimizations improve), and the fact that knownBits was operating at the very edge of MaxRecursionDepth, so changes in common code could push it above the limit.

This commit causes test case failures on s390x (unfortunately hidden as the build bot was already red due to an unrelated issue):
http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/33624

For unsigned conversion, the input is treated as signed here, and the result fixed up by adding 2^N; something along the lines of

(signed)N >= 0 ? signed_to_FP((signed)N) : signed_to_FP(N - 2^N) + (FP)2^N

LGTM, thanks!

In D82322#2120497, @zequanwu wrote:

In D82322#2119815, @uweigand wrote:

It looks like this broke the s390x build bots, since then I'm seeing a lot of ASan and MSan errors:
http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/33485

It seems like all those tests failed because the second CHECK should match output line 4 but missing the line number at the end.
Since the patch passed tests in all other build bots, I wonder if it's because the build bot config is flaky.

It looks like this broke the s390x build bots, since then I'm seeing a lot of ASan and MSan errors:
http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/33485

In D82602#2116747, @spatel wrote:

2. I missed a SystemZ test change in the earlier draft; adding SystemZ reviewers to confirm if that's an improvement.

Hmm, with clang-cl it seems the driver is trying to use this:
Target: s390x-pc-windows-msvc
which of course doesn't exist. Not sure what is supposed to be happening here, but it seems that it's falling back on s390x-linux since on s390x, Linux is currently the only supported OS.

Ping? I'd really appreciate feedback about this ABI issue, in particular from other affected target maintainers ...

Line 4 here fails on s390x but not on other Unix flavors, see: http://lab.llvm.org:8011/builders/clang-s390x-linux/builds/33346/steps/ninja%20check%201/logs/FAIL%3A%20Clang%3A%3Adebug-info-codeview-buildinfo.c

@thakis @uweigand Any ideas would could go wrong here?

In D81671#2094870, @jonpa wrote:

In D81671#2092717, @uweigand wrote:

Shouldn't the check be done inside canUseBlockOperation? I believe the other user (storeLoadCanUseMVC) really needs the same check.

My understanding is that storeLoadCanUseBlockBinary() is used as a pattern matching predicate for block memory operations of AND/OR/XOR. These involve two loads and one store. While canUseBlockOperation() has checks for a pair of a store and the load (of the other address), the second load (to the same address as the store) also needs to be checked, which is what this patch does.

The other user of canUseBlockOperation() is storeLoadCanUseMVC() which however only involves one load and one store, and this is why there is no need for anything else in this case.

This all looks good to me.

Shouldn't the check be done inside canUseBlockOperation? I believe the other user (storeLoadCanUseMVC) really needs the same check.

OK. I agree this can be abandoned now. Thanks!

This looks all good to me now, we should verify that performance results are still good, and then it can go in.

A few cosmetic issues mentioned inline, otherwise this now looks good to me. Thanks!

LGTM.

LGTM, thanks!

It seems that now MachineCSE can remove a few VPERMs also. At least one case was because instead of using an undef source operand, the mask was used (it doesn't help any to replace an undef Ops[1] with Op2 on the last line of getGeneralPermuteNode(), which I first thought). The test case I have reduced (not included) shows that MCSE on trunk fails to remove the second vperm if even though the instructions are near identical. The only difference to the success with this patch is that instead of the reused mask, there is an IMPLICIT_DEF. It looks to me that this is a minor deficiency of MCSE (Two different vregs defined by IMPLICIT_DEF should not stop CSE, I would think).

isZeroOrUndefVector(): The check for the undef vector used to be beneficial for the zero_extend_vector_inreg patch, but the way it looks now (just using a single unpack), it is not needed for anymore (NFC). I think it could be removed or we can keep it here to get the few less vperms...

Two tests - one for each case handled. Not quite sure what happened with vector-constrained-fp-intrinsics.ll - I don't understand why v0 is no longer used, but it seems that this patch begins to change things when the second operand of the VPERM is undefined.

In D78717#2041281, @jonpa wrote:

In D78717#2038456, @serge-sans-paille wrote:

In D78717#2036041, @jonpa wrote:

Normally, I would have expected a pseudo MachineInstruction to be built here with an immediate operand for the size requirement...

That sounds very nice. If you go that way I'll happily update the X86 code accordingly. Or If you point me to some example / doc / code on pseudo MachineInstruction, I can implement that too.

@uweigand : Would you rather use a target pseudo instruction for this rather than using a call with metadata?

Hmm, I still think the read/write_64bit_value change is correct, GCC does read/write 64-bit counter values as lo/hi pairs even on big-endian systems.

In D76705#2029327, @jonpa wrote:

Can we instead move the W->FP mnemonic conversion back into BinaryVRRa/TernaryVRRe instead, so that the OpKey for all these vector instructions consistently holds the FP-reg instruction name? The MemFold-pseudos would then have the same mnemonic as OpKey and MemKey.
... Ah, that still runs into the same name collisions with the MemFold pattern as you pointed out earlier. Sorry for missing that!

Yeah, WFADB:ADBR would map to ADB, and not ADB_MemFoldPseudo which is what we need...

So if we use an extra modifier in the OpKey for those case, then we're basically back to your version of Apr 28, 7:15 PM, except possibly with the explicit mnemonics in the .td file.

Ok, went back to that version plus added the explicit fp_mnemonic fields, which seems nice to me as they eliminate those ugly string substitutions.

In D78486#2020042, @jonpa wrote:

So now I'm wondering instead: why does your new code ever trigger in the first place; why isn't this already handled in GeneralShuffle::add?

I think that be two different cases, where my handling is addressing a *new* permute resulting from combining two operands, when trying to combine (unpack) a third source operand with that first (new) permute.

Hmm, talking about MemFold, I'm wondering about this:
​ // Fused multiply and add/sub need to have the same dst and accumulator reg.

Ah, that still runs into the same name collisions with the MemFold pattern as you pointed out earlier. Sorry for missing that!

In D76705#2020342, @jonpa wrote:

Reverted back to the first version of new instruction classes providing the mapping from W...-reg -> FP-mem instructions.

In D78717#2025239, @jonpa wrote:

Added probing of the tail of dyn-alloca, with an extra check for zero tail, roughly like GCC. It seems it can be assumed that the alloca size will always be a multiple of 8 bytes, or? I think that is necessary, or there might be final probe partially below SP (if tail is e.g. 4 bytes). Also, if the stack-probe size is 0, an infinite loop would result (assert?), but I suppose that would always be noticeable.

In D78486#2014509, @jonpa wrote:

I'm wondering: unless I'm missing something, there's still one specific case where you generate a vperm followed by an unpack (the case where you already had a permute as source). Wouldn't it be preferable to just use a single vperm there as well?

This is the case where the input is a permute that has no other users, which means that it's being replaced. So instead of vperm + vperm, we get a vperm + unpack, which follows the same reasoning of replacing a vperm with a single unpack.

For example, in the case with three source ops, where A and B first are combined with a permute, and now AB and C are to be combined: instead of permuting AB and C, the AB permute is changed so that AB and C can be unpacked.

In D78717#2014532, @jonpa wrote:

gcc seems not to be probing the residual allocation after the loop. However if only two (unrolled) allocations were made, the residual is also probed.

I'm not seeing this, do you have an example?

void large_stack() {
  volatile int stack[2000], i;
  for (i = 0; i < sizeof(stack) / sizeof(int); ++i)
    stack[i] = i;
}

With stack[2000], I see

aghi    %r15,-4096
cg      %r0,4088(%r15)
aghi    %r15,-4072
cg      %r0,4064(%r15)

With stack[8000], I don't see a probe after the loop...

This is missing from the SystemZ back end

In D78486#2012867, @jonpa wrote:

It certainly seems to be an improvement on two benchmarks to do just either one unpack or else a vperm (and not multiple unpacks). In fact, i505.mcf actually regressed (2.5%) when doing two unpacks instead of a vperm (with -ffp-contract=fast). So the initial idea of reducing the number of vperms seems to have been proven wrong - it is better to have a single vperm on the critical path rather than multiple unpacks.

> Is this better than previously?

Not looking at the code so far, but just answering your questions:

Looks good to me now, just one cosmetic comment inline.

In D76705#2007796, @jonpa wrote:

Maybe there should be a test that checks that a live CC value isn't clobbered by introducing an FP-mem instruction, such as ADB...

Hi Jonas, I haven't looked into everything in detail, but first one fundmental question: My understanding was that the current GeneralShuffle code would detect the shuffle you generate for a zero-extend as a case of a MERGE. Later, combineMERGE would detect that one input of the MERGE is a zero vector, and replace the merge by an UNPACKL.

LGTM, thanks!

Thanks!

Just a minor cosmetic suggestion inline. Otherwise the Z ABI parts now all LGTM.

The Z ABI implementation now looks correct to me, except for one corner case: an LLVM-level argument type of f128 is passed via implicit reference. Now, in most cases this is already handled at the clang level, i.e. when you have a "long double" at C source level, you'll already see a pointer type in the LLVM IR instead. However, there are still a few cases where there is a f128 at the LLVM IR level, e.g. as arguments to some compiler builtin / libgcc routines. Those are only transformed to implicit reference in the LLVM back-end, so I believe for completeness this case should also be handled here.

I'm now getting an assertion failure in the LNT test suite:
http://lab.llvm.org:8011/builders/clang-s390x-linux-lnt/builds/17826/steps/test-suite/logs/stdio

LGTM, thanks!

Thanks for running the benchmark, I guess I'm OK with the current implementation then.

Thanks for working on this, @thakis !

Ah, good point. Dimitry, can you prepare an updated patch to implement Jonas' suggestion?

Looking simply at the SystemZ test case change, for the icmp/[zs]ext case (fun1/fun2), we actually need three instructions (compare, load zero, conditional move), so the change seems reasonable.

The more I think about it, the more it seems that the original check has always been somewhat questionable.

Had a look at the vararg handling. Reviewed only the ABI-relevant parts.