- User Since
- Apr 14 2013, 11:48 AM (240 w, 3 d)
Tue, Nov 14
Thu, Nov 9
See two minor inline comments. Otherwise, this now LGTM. Thanks!
Wed, Oct 25
Right. Always coalescing with the copy source of course extends the live range of the source, and thus increases overall register pressure. So it's not a good idea to do this unconditionally.
Tue, Oct 24
Well, the special handling of i128 arguments as such is certainly needed on SystemZ, this is why I added it in the first place in rev. 261325.
Oct 17 2017
The SystemZ parts LGTM. I agree that it's much preferable to have this handled in common code than in the back end ...
Oct 6 2017
OK, this now looks all good to me, thanks!
Oct 4 2017
Have you looked into what's going on with the vec-cmp-cmp-logic-select.ll and vec-cmpsel.ll test cases? I see several of the tests now adding spill/reload code when they didn't before -- ideally this shouldn't happen if the misched does proper register pressure analysis ...
Sep 29 2017
- Quentin approved the interface change
- The SystemZ parts now look good to me
- We have performance runs confirming this does not introduce regressions
Note that as of r314428 I've switched the SystemZ back-end to use a custom expander for ATOMIC_CMP_SWAP_WITH_SUCCESS, which makes use of the condition code value set by the compare-and-swap hardware instruction to completely omit any extra comparison.
Sep 28 2017
Sorry - with the update this fails again, so no update...
Should we make a point to try to handle this somehow?
Sep 27 2017
Sep 25 2017
Sep 22 2017
I looked into making i128 legal when I recently added the 128-bit atomics, but in the end decided against it. Not only would it be a lot of work (since you basically would have to reimplement all the operations on i128 that are currently handled by common code), but it is not clear that we actually always want it.
Sep 21 2017
Looks like this broke the s390x build bots:
This doesn't look in any way target-specific, so I'm wondering why common code doesn't already do that. It seems there is already support for extracting hints from copies in calculateSpillWeightAndHint, why is this not working here?
Sep 20 2017
Looks basically good to me, just a couple of cosmetic comments inline.
The code change looks good to me, but please try to simplify the .mir test case following the suggestions outlined here: https://llvm.org/docs/MIRLangRef.html#simplifying-mir-files
Sep 19 2017
Sep 18 2017
Hmm. Some of those changes look like regressions, we should try to at least understand why this happens. Maybe there's a simple way to fix those again.
Sep 17 2017
Sep 15 2017
As far as SystemZ is concerned, this change should be safe. In our ABI only vector arguments are affected by the IsFixed flags, and there shouldn't be any libcalls with vector arguments.
Thanks for trying this suggestion! See a couple of inline comments.
Aug 21 2017
Aug 18 2017
Aug 17 2017
Oh, and just comment on this:
The getRegAllocationHints implementation makes sense to me. However, I'm wondering if we shouldn't also check for the *destination* register -- you only force one source register to the same class as the other source register, but I think we should check whether *any* of the three registers is already allocated, and then always force the other two to the same class.
Aug 4 2017
Aug 2 2017
Yes, this benefits a real workload. This happens when the improved SimplifyAndInst now simplifies a partial result, which then allows *further* simplification in InstSimplify, in my case in ThreadBinOpOverPHI. This latter simplification doesn't seem to be done anywhere else.
The testcase is extracted from a real-word program. On that program, the transformation (moving some of those operations out of a hot loop) is a significant overall win (about 10% improved performance of the whole program). I agree that this application is a quite special case -- this patch doesn't make much difference to the overall performance of the platform in general.
Sure, it would be possible to add the corresponding transforms to SimplifyOrInst. However, I have been unable to trigger this at all, with any workload I've tried. Should I still add the transforms anyway?
Well, I'd be fine with NewGVN doing this transform. But at least right now, it doesn't appear to be doing so -- running the new test case (@phi_multi in phi.ll) through "opt -O2 --enable-newgvn" does not move the zext/or out of the loop. Is this supposed to happen?
OK, the SystemZ parts look good to me now. Thanks!
Aug 1 2017
Thanks! The SystemZ part now looks very reasonable. Just a couple of final, really just cosmetic comments inline.
Jul 31 2017
A couple more comments on the SystemZ parts, see inline comments.
Jul 28 2017
Jul 27 2017
The SystemZ part looks correct to me, see the inline comment for a style/readability suggestion.
Jul 24 2017
It does indeed seem to be necessary to get an explicit guarantee from common code that scheduling happens in a particular order, otherwise our back-end code may silently break if common code logic happens to change in the future.
Jul 21 2017
I agree this isn't perfect. I also noticed this before, and thought this could be possibly refactored but however also noticed the many different overloaded versions of isLegalAddressingMode() and thought that it could wait as a next step, perhaps. RateFormula() currently only checks the fixup instructions for the offsets, so I am not sure this is a trivial change.
Jul 20 2017
A few comments/questions, looking only at the SystemZ-specific parts.
Jul 17 2017
Jul 5 2017
Jun 30 2017
Jun 26 2017
Jun 23 2017
Jun 20 2017
The new test case fails on SystemZ, making all our build bots red.
Jun 1 2017
May 30 2017
May 29 2017
Yes, this works for me now. Thanks!
It appears this commit broke the EditlineTestFixture.EditlineReceivesSingleLineText unit test on s390x-linux. The test now simply hangs (hanging the whole test suite execution) ...
The problem is that the new test case you added does not contain a -triple argument in the compile command. This means that the compile targets the native architecture on the build system, whatever this is. Since OpenCL support on different architectures may be different (e.g. some may support the cl_khr_fp64 extension by default while others do not), you see the error only on some build architectures.
May 27 2017
I've checked the changes in to the zorg SVN repo, but the new builders don't show up on the build bot web page. Is there anything else I need to do here?
May 26 2017
This seems to have caused a failure in the SystemZ build bot:
Looks like this causes the build bot to fail on SystemZ:
May 24 2017
Can you explain why you want to do this?
May 23 2017
It seems that to identify external symbols, we need to check for *either* non-null Value.SymbolName *or* a SymType of Symbol::ST_Unknown.
This seems to have broken some MCJIT test cases on big-endian ppc64 ... I'll have a look.
Added test case. Note that it is a bit tricky to replicate the exact condition that triggers the bug in a test case. The one attached here seems to fail reliably (before the fix) across different operating system versions on Power, but it still makes a few assumptions (called out in the comments).