Page MenuHomePhabricator

uweigand (Ulrich Weigand)
User

Projects

User does not belong to any projects.

User Details

User Since
Apr 14 2013, 11:48 AM (387 w, 6 d)

Recent Activity

Tue, Sep 15

uweigand accepted D87390: [PowerPC] Pass nofpexcept flag to custom lowered constrained ops.

This LGTM, thanks!

Tue, Sep 15, 9:39 AM · Restricted Project

Fri, Sep 11

uweigand added inline comments to D87510: [SystemZ] Don't emit PC-relative memory accesses to unaligned (packed) symbols..
Fri, Sep 11, 7:04 AM · Restricted Project

Wed, Sep 9

uweigand committed rG1a25133bcdfe: [DAGCombine] Skip re-visiting EntryToken to avoid compile time explosion (authored by uweigand).
[DAGCombine] Skip re-visiting EntryToken to avoid compile time explosion
Wed, Sep 9, 10:14 AM
uweigand closed D86963: [DAGCombine] Skip re-visiting EntryToken to avoid compile time explosion.
Wed, Sep 9, 10:14 AM · Restricted Project
uweigand added a comment to D86963: [DAGCombine] Skip re-visiting EntryToken to avoid compile time explosion.

I'd like to make some progress on this soon, given that this still causes every LNT run on the SystemZ build bot to fail ...

Wed, Sep 9, 9:03 AM · Restricted Project
uweigand added a comment to D86963: [DAGCombine] Skip re-visiting EntryToken to avoid compile time explosion.

Ping.

Wed, Sep 9, 8:55 AM · Restricted Project

Tue, Sep 8

uweigand accepted D87222: [PowerPC] [FPEnv] Disable strict FP mutation by default for PowerPC.

We're finally there, great :-) LGTM.

Tue, Sep 8, 7:03 AM · Restricted Project
uweigand accepted D87220: [PowerPC] Fix STRICT_FRINT/STRICT_FNEARBYINT lowering.

Makes sense to me. As an aside, this code:

Tue, Sep 8, 7:01 AM · Restricted Project, Restricted Project
uweigand accepted D86268: [DAGTypeLegalizer] Handle ZERO_EXTEND of promoted integer in WidenVecRes_Convert..

This LGTM now. Thanks!

Tue, Sep 8, 6:03 AM · Restricted Project

Fri, Sep 4

uweigand accepted D86605: [PowerPC] Expand constrained ppc_fp128 to i32 conversion.

LGTM now, thanks!

Fri, Sep 4, 10:45 AM · Restricted Project
uweigand added a comment to D87130: [SelectionDAGBuilder] Remember to copy the FMF flags in visitBinary()..

I think this makes sense to me, but I want to point out that this partially reverts the commit https://reviews.llvm.org/D46854. I guess there may be other parts of this commit that should be reverted?

The other thing that was removed by that commit was the propagation of the NoNaNs flag from the experimental_vector_reduce_fmax (/fmin) intrinsic to the replacing VECREDUCE_FMAX (/FMIN) node, but I am not sure why...

Fri, Sep 4, 9:52 AM · Restricted Project
uweigand added inline comments to D86605: [PowerPC] Expand constrained ppc_fp128 to i32 conversion.
Fri, Sep 4, 9:46 AM · Restricted Project
uweigand added inline comments to D87115: [FPEnv][X86][SystemZ] Use different algorithms for i64->double uint_to_fp under strictfp to avoid producing -0.0 when rounding toward negative infinity.
Fri, Sep 4, 9:44 AM · Restricted Project
uweigand added inline comments to D86605: [PowerPC] Expand constrained ppc_fp128 to i32 conversion.
Fri, Sep 4, 3:52 AM · Restricted Project
uweigand added a comment to D87130: [SelectionDAGBuilder] Remember to copy the FMF flags in visitBinary()..

I think this makes sense to me, but I want to point out that this partially reverts the commit https://reviews.llvm.org/D46854. I guess there may be other parts of this commit that should be reverted?

Fri, Sep 4, 3:39 AM · Restricted Project
uweigand added inline comments to D87115: [FPEnv][X86][SystemZ] Use different algorithms for i64->double uint_to_fp under strictfp to avoid producing -0.0 when rounding toward negative infinity.
Fri, Sep 4, 3:32 AM · Restricted Project
uweigand added a comment to D86871: [SelectionDAG] Let NSW/NUW flags be cleared by default in call to getNode()..

I may be missing something, but as long as the memoization does a true intersection of the flags per this patch, there should not necessarily be anything broken, or?

Fri, Sep 4, 3:16 AM · Restricted Project

Wed, Sep 2

uweigand added a comment to D86871: [SelectionDAG] Let NSW/NUW flags be cleared by default in call to getNode()..

Thanks for the links, @spatel ! It seems to me that the change in "D46854 - changed code in SelectionDAGBuilder::visit()" is actually not correctly respecting the shared flags semantics. The node returned from getNodeForIRValue, while implementing the translation of this IR, might at the same be be used elsewhere (due to DAG node merging). If that other place does not allow FMF semantics, the flags in the DAG node can still be "undefined", and therefore the new code in "visit" will just replace the flags with a version appropriate only for this IR (which may allow FMF).

Am I missing something here? More generally, how can any after-the-fact setFlags ever be correct, given that the node might have already been merged?

That does seem wrong given the Flags.isDefined() check sitting within intersectWith(). I'm not seeing any regression test failures if we remove that entirely. So let's do that instead of only pushing it below the nsw/nuw intersects?

Wed, Sep 2, 9:10 AM · Restricted Project
uweigand added a comment to D86605: [PowerPC] Expand constrained ppc_fp128 to i32 conversion.

The algorithm used in the unsigned case isn't strict-safe. However, this is exactly the same algorithm that is also used in TargetLowering::expandFP_TO_UINT, where is has been made properly strict-safe in the meantime.

Wed, Sep 2, 8:24 AM · Restricted Project
uweigand added a comment to D86963: [DAGCombine] Skip re-visiting EntryToken to avoid compile time explosion.

Could you share the IR before DAGcombine that triggers the problem? DAGCombine already has limits in a few places that should prevent store merging from causing an explosion in compile-time. Would be good to know where this case slips through.

Wed, Sep 2, 4:51 AM · Restricted Project

Tue, Sep 1

uweigand requested review of D86963: [DAGCombine] Skip re-visiting EntryToken to avoid compile time explosion.
Tue, Sep 1, 10:36 AM · Restricted Project
uweigand added a comment to D86871: [SelectionDAG] Let NSW/NUW flags be cleared by default in call to getNode()..

Thanks for the links, @spatel ! It seems to me that the change in "D46854 - changed code in SelectionDAGBuilder::visit()" is actually not correctly respecting the shared flags semantics. The node returned from getNodeForIRValue, while implementing the translation of this IR, might at the same be be used elsewhere (due to DAG node merging). If that other place does not allow FMF semantics, the flags in the DAG node can still be "undefined", and therefore the new code in "visit" will just replace the flags with a version appropriate only for this IR (which may allow FMF).

Tue, Sep 1, 9:29 AM · Restricted Project
uweigand accepted D86595: [PowerPC] Handle STRICT_FSETCC(S) in more cases.

This LGTM now, thanks!

Tue, Sep 1, 12:47 AM · Restricted Project

Mon, Aug 31

uweigand added a comment to D86871: [SelectionDAG] Let NSW/NUW flags be cleared by default in call to getNode()..

Interesting. I agree that the flags should always be reset here. In fact, I'm questioning why we should have that "isDefined" logic in the first place. It seems to me that any node with "undefined" flags can only be used correctly if any setting of the flags would be semantically correct -- but then we may just as well use the all-zero setting of the flags anyway.

Mon, Aug 31, 6:59 AM · Restricted Project

Fri, Aug 28

uweigand added inline comments to D86268: [DAGTypeLegalizer] Handle ZERO_EXTEND of promoted integer in WidenVecRes_Convert..
Fri, Aug 28, 6:17 AM · Restricted Project

Wed, Aug 26

uweigand requested changes to D86595: [PowerPC] Handle STRICT_FSETCC(S) in more cases.
Wed, Aug 26, 8:48 AM · Restricted Project

Aug 21 2020

uweigand accepted D81669: [PowerPC] Support constrained fp operation for scalar sitofp/uitofp.

OK, this now looks all good to me as well. Thanks!

Aug 21 2020, 5:03 AM · Restricted Project

Aug 20 2020

uweigand added a comment to D81669: [PowerPC] Support constrained fp operation for scalar sitofp/uitofp.

A few comments inline, otherwise this looks good.

Aug 20 2020, 8:47 AM · Restricted Project
uweigand accepted D81918: [PowerPC] Support lowering int-to-fp on ppc_fp128.

LGTM now. Thanks!

Aug 20 2020, 8:34 AM · Restricted Project

Aug 19 2020

uweigand accepted D81537: [PowerPC] Support constrained fp operation for scalar fptosi/fptoui.

This LGTM now. Thanks!

Aug 19 2020, 8:10 AM · Restricted Project

Aug 13 2020

uweigand added inline comments to D81918: [PowerPC] Support lowering int-to-fp on ppc_fp128.
Aug 13 2020, 6:00 AM · Restricted Project
uweigand added inline comments to D81537: [PowerPC] Support constrained fp operation for scalar fptosi/fptoui.
Aug 13 2020, 5:00 AM · Restricted Project

Aug 12 2020

uweigand accepted D85822: [Sanitizer] Fix segfaults during unwinding on SystemZ.

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

Aug 12 2020, 11:43 AM · Restricted Project

Aug 10 2020

uweigand added a comment to D81537: [PowerPC] Support constrained fp operation for scalar fptosi/fptoui.

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?

Aug 10 2020, 10:27 AM · Restricted Project

Aug 4 2020

uweigand added a comment to D81537: [PowerPC] Support constrained fp operation for scalar fptosi/fptoui.

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.

Aug 4 2020, 5:50 AM · Restricted Project

Aug 3 2020

uweigand accepted D81906: [CodeGen] Expand float operand for STRICT_FSETCC/STRICT_FSETCCS.

This LGTM now.

Aug 3 2020, 3:49 AM · Restricted Project
uweigand added a comment to D84835: [APFloat] Fix incorrect fptrunc rouning when from semantics exp lower than minimal exp of target semantics.

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

Aug 3 2020, 3:40 AM · Restricted Project

Jul 30 2020

uweigand added a comment to D84764: Fix computeHostNumPhysicalCores() for Linux on POWER and Linux on Z.

Sure, of course assuming the test still passes.

Jul 30 2020, 2:40 AM · Restricted Project

Jul 29 2020

uweigand accepted D84764: Fix computeHostNumPhysicalCores() for Linux on POWER and Linux on Z.

LGTM for SystemZ as well.

Jul 29 2020, 10:38 AM · Restricted Project

Jul 24 2020

uweigand committed rG7f003957bfcd: [SystemZ] Implement __builtin_eh_return_data_regno (authored by uweigand).
[SystemZ] Implement __builtin_eh_return_data_regno
Jul 24 2020, 1:29 AM
uweigand closed D84341: Implement __builtin_eh_return_data_regno for SystemZ.
Jul 24 2020, 1:29 AM · Restricted Project

Jul 23 2020

uweigand committed rG68a80a4436c6: [SystemZ] Ensure -mno-vx disables any use of vector features (authored by uweigand).
[SystemZ] Ensure -mno-vx disables any use of vector features
Jul 23 2020, 6:35 AM

Jul 22 2020

uweigand accepted D84341: Implement __builtin_eh_return_data_regno for SystemZ.

LGTM with the format fixes.

Jul 22 2020, 10:29 AM · Restricted Project

Jul 21 2020

uweigand accepted D83654: [PowerPC] Support constrained vector fp/int conversion.

This LGTM now.

Jul 21 2020, 12:46 AM · Restricted Project

Jul 17 2020

uweigand added inline comments to D83654: [PowerPC] Support constrained vector fp/int conversion.
Jul 17 2020, 3:47 AM · Restricted Project
uweigand requested changes to D81918: [PowerPC] Support lowering int-to-fp on ppc_fp128.

See inline comments about chain handling.

Jul 17 2020, 3:43 AM · Restricted Project

Jul 16 2020

uweigand accepted D81727: [PowerPC] Support constrained fp operation for setcc.

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.

Jul 16 2020, 5:19 AM · Restricted Project

Jul 14 2020

uweigand added inline comments to D83654: [PowerPC] Support constrained vector fp/int conversion.
Jul 14 2020, 4:00 AM · Restricted Project

Jul 12 2020

uweigand added a comment to D80833: [CodeView] Add full repro to LF_BUILDINFO record.

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 ?

Jul 12 2020, 3:29 PM · Restricted Project, Restricted Project

Jul 10 2020

uweigand closed D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute.

Committed as 4c5a93bd58bad70e91ac525b0e020bd5119a321a.

Jul 10 2020, 5:12 AM · Restricted Project
uweigand committed rG4c5a93bd58ba: [ABI] Handle C++20 [[no_unique_address]] attribute (authored by uweigand).
[ABI] Handle C++20 [[no_unique_address]] attribute
Jul 10 2020, 5:01 AM

Jul 9 2020

uweigand updated the diff for D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute.

Drop AllowNoUniqueAddress parameter; address review comment.

Jul 9 2020, 12:50 AM · Restricted Project

Jul 8 2020

uweigand committed rGcca8578efab0: [SystemZ] Allow specifying integer registers as part of the address calculation (authored by uweigand).
[SystemZ] Allow specifying integer registers as part of the address calculation
Jul 8 2020, 9:21 AM
uweigand closed D83251: [SystemZ] Allow specifying integer registers as part of the address calculation.
Jul 8 2020, 9:20 AM · Restricted Project
uweigand added a comment to D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute.

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.

Jul 8 2020, 7:10 AM · Restricted Project
uweigand added a comment to D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute.

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.

Jul 8 2020, 7:08 AM · Restricted Project
uweigand added inline comments to D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute.
Jul 8 2020, 7:07 AM · Restricted Project
uweigand updated the diff for D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute.

Handle array of empty records correctly.

Jul 8 2020, 6:59 AM · Restricted Project
uweigand accepted D83376: [Legalizer] Fix wrong operand in split vector helper.

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

Jul 8 2020, 5:00 AM · Restricted Project

Jul 7 2020

uweigand updated the diff for D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute.

Rebased against mainline.

Jul 7 2020, 11:26 AM · Restricted Project
uweigand committed rG80a1b95b8e72: [SystemZ ABI] Allow class types in GetSingleElementType (authored by uweigand).
[SystemZ ABI] Allow class types in GetSingleElementType
Jul 7 2020, 10:57 AM
uweigand abandoned D74601: SystemZ target - Incorrect code generated for accessing thread_local variables when high-word feature is enabled.
Jul 7 2020, 5:27 AM · Restricted Project
uweigand commandeered D74601: SystemZ target - Incorrect code generated for accessing thread_local variables when high-word feature is enabled.
Jul 7 2020, 5:27 AM · Restricted Project
uweigand added a comment to D74601: SystemZ target - Incorrect code generated for accessing thread_local variables when high-word feature is enabled.

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

Jul 7 2020, 5:27 AM · Restricted Project
uweigand added a comment to D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute.

Another ping ...

Jul 7 2020, 5:25 AM · Restricted Project
uweigand accepted D83251: [SystemZ] Allow specifying integer registers as part of the address calculation.

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

Jul 7 2020, 2:16 AM · Restricted Project

Jun 30 2020

uweigand added a comment to D82622: [DWARFYAML][debug_info] Replace 'InitialLength' with 'Format' and 'Length'..

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.

Jun 30 2020, 9:11 AM · Restricted Project, Restricted Project
uweigand added a comment to D82602: [SelectionDAG] don't split branch on logic-of-vector-compares.

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.

Jun 30 2020, 8:06 AM · Restricted Project
uweigand committed rGe9c6b63d4a16: [SystemZ] Simplify knownbits.ll test (authored by uweigand).
[SystemZ] Simplify knownbits.ll test
Jun 30 2020, 7:35 AM
uweigand added a comment to D82622: [DWARFYAML][debug_info] Replace 'InitialLength' with 'Format' and 'Length'..

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

Jun 30 2020, 5:55 AM · Restricted Project, Restricted Project
uweigand requested changes to D81727: [PowerPC] Support constrained fp operation for setcc.
Jun 30 2020, 3:14 AM · Restricted Project
uweigand requested changes to D81906: [CodeGen] Expand float operand for STRICT_FSETCC/STRICT_FSETCCS.
Jun 30 2020, 3:14 AM · Restricted Project
uweigand added a comment to D81918: [PowerPC] Support lowering int-to-fp on ppc_fp128.

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
Jun 30 2020, 2:08 AM · Restricted Project
uweigand accepted D82794: [SystemZ] Add NoMerge MIFlag.

LGTM, thanks!

Jun 30 2020, 12:30 AM · Restricted Project

Jun 29 2020

uweigand updated subscribers of D82322: [ASan][MSan] Remove EmptyAsm and set the CallInst to nomerge to avoid from merging..

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.

Jun 29 2020, 10:48 AM · Restricted Project
uweigand added a comment to D82322: [ASan][MSan] Remove EmptyAsm and set the CallInst to nomerge to avoid from merging..

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

Jun 29 2020, 6:26 AM · Restricted Project

Jun 26 2020

uweigand added a comment to D82602: [SelectionDAG] don't split branch on logic-of-vector-compares.
  1. I missed a SystemZ test change in the earlier draft; adding SystemZ reviewers to confirm if that's an improvement.
Jun 26 2020, 8:44 AM · Restricted Project
uweigand added inline comments to D81537: [PowerPC] Support constrained fp operation for scalar fptosi/fptoui.
Jun 26 2020, 5:56 AM · Restricted Project

Jun 25 2020

uweigand added inline comments to D81537: [PowerPC] Support constrained fp operation for scalar fptosi/fptoui.
Jun 25 2020, 9:07 AM · Restricted Project

Jun 24 2020

uweigand added inline comments to D81537: [PowerPC] Support constrained fp operation for scalar fptosi/fptoui.
Jun 24 2020, 7:31 AM · Restricted Project

Jun 23 2020

uweigand added a comment to D80833: [CodeView] Add full repro to LF_BUILDINFO record.

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.

Jun 23 2020, 7:58 AM · Restricted Project, Restricted Project
uweigand added a comment to D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute.

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

Jun 23 2020, 1:32 AM · Restricted Project
uweigand added a comment to D80833: [CodeView] Add full repro to LF_BUILDINFO record.

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?

Jun 23 2020, 1:32 AM · Restricted Project, Restricted Project

Jun 16 2020

uweigand accepted D81671: [SystemZ] Bugfix in storeLoadCanUseBlockBinary().

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.

Jun 16 2020, 1:04 AM · Restricted Project

Jun 15 2020

uweigand accepted D78644: [LSan] Enable for SystemZ.

This all looks good to me.

Jun 15 2020, 9:45 AM · Restricted Project, Restricted Project
uweigand added a comment to D81671: [SystemZ] Bugfix in storeLoadCanUseBlockBinary().

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

Jun 15 2020, 6:28 AM · Restricted Project

Jun 10 2020

uweigand created D81583: Update SystemZ ABI to handle C++20 [[no_unique_address]] attribute.
Jun 10 2020, 9:17 AM · Restricted Project

Jun 5 2020

uweigand added a comment to D78488: [SystemZ] Emit VLLEZ from tryShuffleWithZeroVec().

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

Jun 5 2020, 8:52 AM · Restricted Project
uweigand accepted D78486: [SystemZ] Expand vector zero-extend into a shuffle..

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

Jun 5 2020, 8:52 AM · Restricted Project
uweigand accepted D78717: [SystemZ] Implement -fstack-clash-protection.

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

Jun 5 2020, 8:51 AM · Restricted Project, Restricted Project

Jun 4 2020

uweigand accepted D80329: [SVE] Eliminate calls to default-false VectorType::get() from SystemZ.

LGTM.

Jun 4 2020, 2:40 AM · Restricted Project

May 18 2020

uweigand accepted D79925: [SystemZ] Eliminate the need to create a zero vector by reusing the mask..

LGTM, thanks!

May 18 2020, 12:26 PM · Restricted Project
uweigand added a comment to D79925: [SystemZ] Eliminate the need to create a zero vector by reusing the mask..

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.

May 18 2020, 8:34 AM · Restricted Project
uweigand added a comment to D78717: [SystemZ] Implement -fstack-clash-protection.

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?

May 18 2020, 5:52 AM · Restricted Project, Restricted Project

May 12 2020

uweigand added a comment to D49132: Fix gcov profiling on big-endian machines.

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.

May 12 2020, 1:02 AM

May 11 2020

uweigand accepted D76705: [SystemZ] Improve foldMemoryOperandImpl: vec->FP conversions.

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.

May 11 2020, 8:34 AM · Restricted Project

May 8 2020

uweigand added a comment to D78486: [SystemZ] Expand vector zero-extend into a shuffle..

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.

May 8 2020, 12:52 PM · Restricted Project
uweigand added a comment to D76705: [SystemZ] Improve foldMemoryOperandImpl: vec->FP conversions.

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

May 8 2020, 12:52 PM · Restricted Project
uweigand added a comment to D76705: [SystemZ] Improve foldMemoryOperandImpl: vec->FP conversions.

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

May 8 2020, 12:20 PM · Restricted Project
uweigand added a comment to D76705: [SystemZ] Improve foldMemoryOperandImpl: vec->FP conversions.

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

May 8 2020, 9:37 AM · Restricted Project