- User Since
- Apr 14 2013, 11:48 AM (270 w, 1 d)
Fri, Jun 8
Wed, Jun 6
Otherwise, this LGTM.
Fri, Jun 1
This version now correctly implements target support for STRICT floating-point nodes using a MMO to represent the floating-point exception status. (Note that targets can and should additionally use a register use dependency on *all* floating-point instructions to represent floating-point control state, e.g. rounding mode and exception trap flags.)
Working version of the patch using memory operands.
Wed, May 30
Tue, May 29
(Failed) attempt to handle strict and regular FP operations without duplicating patterns ...
Is there a reason for dropping the "FP" from "StrictFP" in the method name? I think it would be better to keep it (i.e. use "getStrictFPOperationAction"), in particular since there are already other public methods in this area that use "StrictFP", like isStrictFPOpcode and MutateStrictFPToFP.
Thu, May 24
Wed, May 23
Tue, May 22
In addition to what Andrew said, there's another reason not to mutate this early: sooner or later, we'll need to give the target the chance to actually model strict operations differently (e.g. via more precise tracking of the FP status bit dependencies). At this point, the information which operations were strict must be preserved until the MI level anyway.
May 19 2018
Apr 30 2018
So, in effect, never do the IPRA callee-saved optimization for R11, because the caller *could* use it as frame pointer? That's certainly the conservative solution.
But we do not want to force use of a frame pointer, in general this just reduces performance for no reason on Z. Some functions require a frame pointer (those that do dynamic allocas), but most do not.
This looks like a generic problem to me, many platforms have registers that are reserved only in some functions but not others. If function A where a register is reserved calls function B where the register is not reserved, something must ensure that the register is preserved across the call to B. Usually, this is the case because such registers need to be callee-saved. But if function B is optimized via IPRA to not save the register, we have a problem ...
Well, usually this is handled correctly by the isPhysRegModified call in SystemZFrameLowering::determineCalleeSaves. (The only reason why we even need the extra hasCalls check is that the call instruction is currently not at all stages modeled correctly to show that R14 is clobbered.)
I do not believe we need to do anything special with R11. This is only special if it is used as frame pointer, in which case the code before already adds it as caller-saved:
Apr 26 2018
The test case looks fine to me, thanks.
Apr 24 2018
Apr 12 2018
Apr 5 2018
Mar 15 2018
The original bug in PR 36410 is now fixed in r327622, so I won't pursue this attempt any further.
Mar 14 2018
Updated comment as requested.
Mar 12 2018
Would it be an option to only hoist debug info instructions where both variable and value match (just like the existing code), but then not attempt to merge the !dbg locations, but instead just hoist both copies of the original instructions with both the original locations (like in this patch)?
Mar 9 2018
Fix the local scope bug.
Ah, I see. Looks like the problem is that the DIScope -> getScope() loop can actually leave the function scope and go up all the way to file scope. This is not a good idea here. I think this needs to be restricted to DILocalScope instead. I'll update the patch.
Sorry, I didn't see your comment before checking this in. I believe you're right, and this can cause problems --- I've now reverted and will have another look.
Updated test case as requested.
Mar 8 2018
This doesn't seem to make any difference for SystemZ. Adding Hal, Kit, and Nemanja for PowerPC ...
Mar 2 2018
Feb 27 2018
This fixed the problem, thanks!
Feb 26 2018
This breaks the SystemZ build bots, because the new debug-empty-source.s test case uses a "nop" instruction, which does not exist (using this name) on SystemZ.
I'm not sure I understand why we should keep two debug intrinsics around in just this case, and not in other cases where the original IR has differing debug intrinsics (e.g. for different variables)?
Feb 23 2018
Feb 16 2018
I've run into an internal compiler error that seems caused by this behavior of getMergedLocation: https://bugs.llvm.org/show_bug.cgi?id=36410
Feb 14 2018
Ah, so this is a problem when some processor models use itineraries and others don't? In that case, the change looks good to me ...
Feb 9 2018
Checked in as rev. 324726. Thanks!
Feb 8 2018
Hi Arnold, I only now noticed this commit. I wasn't aware there were any SystemZ back-end issues relating to swifterror support -- can you elaborate? Is there anything I should be doing in the back-end to fix this?
Jan 31 2018
Yes, that makes sense to me. It also agrees with the layout of bitfields on (all currently supported) big-endian platforms, so that's another argument for it.
Jan 30 2018
I've been talking to Jonas about the big-endian question, and I now think that there may actually be open questions about what the layout should be.
Jan 22 2018
Jan 19 2018
OK, I've now checked in both changes: enable long tests in the clang-s390x-linux builder, and run this large test only when long tests are enabled.
I see that the build bot switched off long tests some time ago. I'll try to switch this on again for at least one of the SystemZ builders.
We could also move the test to the Large directory with the other large-branch tests; this is not run by default but only when explicitly requested.
Jan 18 2018
Superseded by https://reviews.llvm.org/D41798
Jan 16 2018
Jan 15 2018
Jan 8 2018
This patch seems to be identical to the one I proposed here:
Dec 6 2017
This has caused build bot failures on SystemZ (and others):
Dec 5 2017
The SystemZ changes LGTM. Given that Quentin approved the common part, this should be fine.
Dec 4 2017
For consistency, the other variants of the tbegin intrinsic (int_s390_tbegin and int_s390_tbegin_nofloat) should likewise get the IntrWriteMem flag.
The TwoAddress fix has been committed, but the intrinsics flags fixing seem to require some work so I am not sure if we really want to wait with these changes until that is done?
Dec 1 2017
These changes (at the code level) look reasonable to me now. (There is one mayStore on a STOC instruction that is probably still superfluous now.)
Nov 30 2017
Thanks for providing the diff. I noticed two problems, also annotated in the detailed comment:
- You added mayLoad to a number of load-conditional instructions that actually don't access memory (reg-reg or reg-immed instructions)
- The guarded-storage instructions should really have the side effects flag (I overlooked this in my previous review).
Nov 29 2017
This LGTM now, thanks!
Nov 28 2017
Actually, I take it back, sorry ...