User Details
- User Since
- Jan 23 2015, 9:38 AM (388 w, 5 d)
Yesterday
I think this is the right thing to do regardless of whether it affects CTR loops or not. The FMA is never faster with SPE and that should be marked as such. But yes, we still need a test case.
Tue, Jul 5
Please run clang-format, rebase and re-upload. It doesn't apply cleanly to ToT.
LGTM. Thanks for fixing this.
Mon, Jul 4
As tends to be the case with huge patches such as this, it is difficult to get this patch to converge and it still needs a bit of work. But I think it is pretty close overall.
Fri, Jul 1
Wed, Jun 29
"Made a new phabricator review because of git issues" is not an appropriate description of a review/revision.
I am ok with this change overall, I just have a couple of questions about naming of the option.
Tue, Jun 28
Is it possible to add a testcase that fails to verify or otherwise shows that a live range is computed incorrectly?
Ah, this is a good point. I guess we didn't consider that we may have a bitcast of a bitcast. Thanks for fixing this. LGTM.
Fri, Jun 24
Fri, Jun 17
LGTM. Thanks for fixing this.
Thu, Jun 16
LGTM. Thanks.
LGTM. Thanks for the fix and all the updates.
Wed, Jun 15
LGTM. Thanks for the fix.
Tue, Jun 14
If this IR is compiled with -mattr=+quadword-atomics, we emit #STQX_PSEUDO which is definitely not what we want:
%struct.StructA = type { [16 x i8] }
Wed, Jun 8
These suffixes were always part of the code and the mentioned changeset removed it without any justification. The burden of providing this answer should lie with the author of the change that removed this (i.e. @tbaeder). Here at IBM, we are not aware of any Redhat release that does not have this suffix. Namely, all of our RH PowerPC machines with all the toolset versions and distro versions we have available have the suffix. So we are ill equipped to answer this question. I assume that Timm is aware of situations where this isn't part of the path.
The original toolchain detection added root/usr to the paths which was certainly necessary on PowerPC. Since that suffix is removed in this patch, our Redhat buildbot is now broken (https://lab.llvm.org/buildbot/#/builders/57/builds/18577). @quinnp has posted an update to this at https://reviews.llvm.org/D127310.
Tue, Jun 7
This was never committed because it turned out not to be beneficial. Abandoning.
Jun 6 2022
Jun 2 2022
LGTM.
Jun 1 2022
In any case, this works if the offset fits but not if it doesn't. So we're just kicking the can down the road a little bit. We need to add the X-Form pseudo to the ImmToIdxMap and expand it at the appropriate time.
May 31 2022
May 25 2022
FWIW, this test fails exactly the same way on my MacBook if I use llvm-nm on llvm-config which produces shorter output (although it is successful in its current form). So this seems to be completely pervasive and not at all restricted to PowerPC (or even Linux).
May 24 2022
The more analysis we do in FastISel, the less "fast" it becomes. And the tradeoff here is simply a nop after a call which hardly seems like a concern at -O0.
This seems like a rather invasive refactoring for removing a nop at -O0. I am not particularly in favour of this. Not only is a nop essentially trivial when it comes to performance (and even code size), but this is restricted to unoptimized code gen for which it isn't really reasonable to expect performant code.
May 18 2022
May 17 2022
May 10 2022
May 9 2022
Why not also fix this in the front end so that we allow the builtin on the A2 CPU as well (since it's supported)?
May 8 2022
I am sorry that I didn't get to this within a more reasonable time. This LGTM for PPC and thanks for the patch.
This is fine from a PPC perspective. What would make more of an impact would be the ability to common partial immediate materializations such as happen in prologue/epilogue when a stack frame is very large. Perhaps this can be extended to do that in the future. It would eliminate things like:
lis 11, 16 ori 11, 11, 256 stxvx 53, 31, 11 # 16-byte Folded Spill lis 11, 16 ori 11, 11, 272 stxvx 54, 31, 11 # 16-byte Folded Spill lis 11, 16 ori 11, 11, 288 stxvx 55, 31, 11 # 16-byte Folded Spill lis 11, 16 ori 11, 11, 304 stxvx 56, 31, 11 # 16-byte Folded Spill
Of course, that would involve changing the subsequent ori instructions to addi and would likely be very PPC-specific. I just bring it up here in case SystemZ has the same issue.
May 7 2022
May 3 2022
May 2 2022
Thank you for fixing this!
Apr 28 2022
Apr 27 2022
Apr 26 2022
LGTM. Thanks for the fix.
This can go in as it is. Both Clang and GCC define this macro on PPC. However, it will mean that we don't define these even for compilations with -mabi=ieeelongdouble and -mlong-double-64. If it matters that this works when long double means IEEE 754-R Quad Precision or simply double precision, you could guard it with something like the following
#if defined(__PPC__) && defined(__LONG_DOUBLE_128__) && !defined(__LONG_DOUBLE_IEEE128__)
Apr 25 2022
Apr 21 2022
@daltenty Can you please run this with the same config as the bot on one of our AIX machines but be sure to pass -i if it's make or -k 0 if it's ninja?
FWIW, this is the reduced test from the PPC bot failure:
; ModuleID = 'bugpoint-reduced-simplified.bc' source_filename = "macroblock-e4628c.c" target datalayout = "e-m:e-i64:64-n32:64-S128-v256:256:256-v512:512:512" target triple = "powerpc64le-unknown-linux-gnu"
Apr 20 2022
Also, please run clang-format on the changes.
This is still causing failures when building test-suite:
https://lab.llvm.org/buildbot/#/builders/105/builds/24292
Apr 19 2022
Apr 18 2022
Committed in https://reviews.llvm.org/rG315d79213025
Apr 13 2022
While I certainly appreciate the work to refactor this behemoth of a function, I am about as lukewarm on how much of a readability improvement this is. If all the targets are being similarly refactored, I won't stand in the way of doing the same to PPC. So if you get the approval for the other targets, we'll go ahead with PPC as well.
Sorry, I had some comments that I haven't submitted because I had not gotten through the entire patch. I am not sure if they all still apply but thought I'd submit them before restarting the review.
Apr 7 2022
Please add a test case that would cause a failure prior to this patch due to the argument being emitted more than once (i.e. the test case that prompted this patch). If that is already added and I just missed it, please add a note to the test case along the lines of:
// The argument expression must not be emitted multiple times.
Apr 6 2022
I am not familiar with the FreeBSD-specific stuff, but there is nothing I see any issues with here.
LGTM.
Mar 31 2022
LGTM.
Mar 21 2022
Seems like we should look at the caller side as well since we might have a similar issue.
Mar 14 2022
Mar 9 2022
This certainly fixes all the lsan issues on Ubuntu 18.04, leaving only one failure in the CRT test:
CRT-powerpc64le-linux :: dso_handle.cpp
Mar 7 2022
LGTM other than the test cases that need to be improved.
Mar 4 2022
LGTM.
LGTM.
Feb 25 2022
Feb 23 2022
LGTM.
Feb 22 2022
Feb 17 2022
Feb 16 2022
Feb 15 2022
LGTM.
LGTM. Thanks for fixing this.
Feb 11 2022
LGTM.