- User Since
- Apr 14 2013, 11:48 AM (335 w, 4 d)
Fri, Sep 13
This LGTM now, thanks!
Thu, Sep 12
I agree with making isHighReg a static function in SystemZRegisterInfo. I don't see why there would be any issue with a static member function, as long as it has no persistent state. The only issue with multi-threaded programs would be persistant state, e.g. a static member variable or a static variable within the member function. Arguments and (nonstatic) local variables are fine.
Wed, Sep 11
My understanding was that this is a temporary restriction; in the end, the goal is to allow mixing constrained an non-constrained operations, once we have the necessary barriers. Did I misunderstand this?
I interpret this to mean that this patch handles some additional ~1000 cases. This seems to mean that the compare is now folded with the load load instead of with the jump, like 'lg; cgrje' => 'cg; je'. I am not sure this is really better, then? Would it be worth the trouble of adding some kind of live-in lists check before regalloc for non-allocatable phys reg(s)?
I thought it was a bit simpler and cleaner to keep the expandLOCRPseudo() and expandSELRPseudo() in SystemZInstrInfo. Is it ok to use "public:" and "private:" like this, or should they be moved instead?
Mon, Sep 9
Fixed comment typo.
Sun, Sep 8
Fri, Sep 6
Thu, Sep 5
Anyway, given that it's already been this way, this patch LGTM. We can have a look at the indirect branch vs. scheduling question later.
Wed, Sep 4
So this removes the attempted handling of constant arguments. I agree with @efriedma that it doesn't make sense to just do this small optimization; we should really constant fold.
Remove attempt to handle constant arguments
Ah, good point. Currently, constrained floating-point intrinsics are never constant-folded. In general, this would likely be wrong anyway, since we might not know the current rounding mode that applies.
With the current form of the patch, do we see any noticeable regression due to no additional compares after addition? If no, the patch would be OK; if yes, we'll probably have to add support for AL etc. first.
Tue, Sep 3
Mon, Sep 2
Thu, Aug 22
Wed, Aug 21
Aug 9 2019
Aug 6 2019
Thanks, Hal. I agree we'll need to clean up the "fallback" Expand logic once targets have moved to actually supporting strict FP nodes.
Aug 5 2019
Should we move ahead with this? I believe this is still a pre-req for D63782 ...
Jul 26 2019
Jul 25 2019
Jul 24 2019
I've now posted a patch implementing something along those lines here: https://reviews.llvm.org/D65226
Jul 22 2019
I've now reverted the patch on the LLVM 9 branch as well (r366690).
Jul 21 2019
Jul 19 2019
I've been thinking about the getStrictFPOperationAction issue. I believe this function makes no sense at all and really should go away. Instead, the strict opcodes should use their own operation actions just like everything else. In current code, those should now be set up in a reasonable manner: they all default to Expand, unless the target overrides them (to whatever makes sense).
Looks like this patch was reverted on mainline now.
Jul 18 2019
I tried to run it on my Intel system for comparison, and the test case is failing there too:
Just a quick heads-up: the new test case is failing on SystemZ. I'm not quite sure why, but the build bots show this error:
Jul 16 2019
Just to clarify one thing: even the current implementation, before this patch, does not guarantee the relative order of FP instructions and memory instructions is unchanged. So even the current implementation may perform the reschedule your comment mentions. This patch would add the additional option of also changing the relative order of the two strict_fmul operations.
Jul 15 2019
Ping? It would be good to get this in LLVM 9 ...
Jul 12 2019
I agree it's a bit hacky, but I don't see any better solution either. In any case, the change is not wrong -- everything rejected should be rejected. Given that this fixes a real problem, I'm fine with the patch. LGTM.
Thanks for the quick revert! The new patch looks reasonable to me.
Jul 11 2019
This commit broke codegen on SystemZ, we're now failing both the test-suite and the multistage bootstrap. Unfortunately this wasn't detected earlier since the SystemZ build bot was down.
Jul 9 2019
This scheduler generally only operates on single basic blocks, so it would not move anything outside of a test.
Jul 5 2019
Unfortunately, I think this gets the semantics wrong, because "carry" and "borrow" use *different* CC values on SystemZ.
Jun 28 2019
Jun 27 2019
Jun 26 2019
This seems to have broken test-suite build bots?
Jun 24 2019
Jun 19 2019
I'm now getting test suite failures like this:
Jun 18 2019
Jun 9 2019
Jun 7 2019
At this point, this looks good to me. Thanks!
Jun 6 2019
Jun 5 2019
Addressed review comments and updated to mainline changes.
Jun 4 2019
Looking mostly good now, thanks! Just a few remaining questions/comments inline ...
Jun 3 2019
I'm a bit confused about the mapping logic. For the case of e.g. ADD, we today have
AR ---> gets mapped to A by getMemOpcode
ARK --> no mapping via getMemOpcode
May 31 2019
I think this is good for now. We can work on further improving folded reloads as a follow-on. LGTM.
I agree this still checks the same thing, so the patch LGTM.
May 29 2019
As of r361920, the SystemZ build bots are green again. Thanks, Eric!