- User Since
- May 5 2018, 9:37 AM (54 w, 2 d)
Handle pre-increment case as well, improve comment, rebase over additional tests.
Sun, May 19
Wed, May 15
Just to be clear, I didn't mean to say that we shouldn't do this, just that it is not guaranteed to be a win in the end. If it's possible to do some end-to-end performance testing with this patch to make sure that it doesn't come with unexpected regressions, I see no reason not to move forward with this change. The problem with assumes is a long-standing one and I don't think it has any simple solution.
LGTM. In the future, it might be nice to generalize this as a getConstFloatRange() function (that would be [-INF, C] for minnum and [C, INF] for maxnum) and fold the comparison based on that.
Tue, May 14
Mon, May 13
A few last issues...
Add -asm-verbose=false, move CHECK lines, remove tabs.
Sat, May 11
This one can probably be abandoned in favor of D60261?
Reverse ping. Apart from the previous comment this looks good, and I saw you already added some tests for that as well :)
Add the last missing binary op to ConstantRange: sdiv.
Rereading the ML discussion just now, it looks like the consensus is to go with this option. Move forward?
This looks good, but please also update existing tests that specify three types in the intrinstic name. Here's ones I found:
Fri, May 10
Rebase over D61744.
I'm going to drop this for now -- the original motivation here went away with the signed intersection support. These changes still make sense to improve icmp instsimplify, but are also not super valuable in that instcombine can handle it.
Still needs some duplicates CHECKs removed, per comments above.
@spatel What do you think about going for the following abstraction instead? What I have in mind here is to make this also reusable for operations other than binaryOp(), such as uadd_sat() and friends.
Thu, May 9
Here's a slightly cleaned up version of the reproducer:
Vector op legalization will automatically recursively legalize the returned SDValue, but we need to take care of the other results ourselves.
Wed, May 8
@uabelho Thanks for the reproducer. I think the problems is that when we thread a comparison over a phi in instsimplify, we retain the original context instruction, while (I think) we should be using the terminator of the incoming value as context.
@aemerson Thanks for the report! From a quick look, looks like the usubsat expands to usubo, which is then expanded during op legalization, while it needs to be expanded during vector op legalization. We're doing that for umulo and smulo, but apparently not for uaddo/saddo and usubo/ssubo. I'll prepare a patch.
I've reverted this change for now (rL360260).
Tue, May 7
Mon, May 6
Abandoning this one, don't think there's anything worthwhile left here.
Use both pre-inc and post-inc AddRec nowrap flags.
Sat, May 4
Thu, May 2
This is a good idea in theory, but I'm somewhat concerned that this will backfire in practice because assumes often pessimize optimization. For example LLVM has a really bad offender where it inserts assumes for alignment annotations during inlining -- thankfully there's an option to disable it, because it's a major perf regression otherwise.
Wed, May 1
I suspect that it's not safe to do this in this form: While the shift instruction on the target might be well defined, this is still operating on ISD opcodes that consider out-of-bounds shifts to be undef. After you have eliminated the check, some other combine could come along and optimize the shift to undef based on that.
Mon, Apr 29
Sun, Apr 28
Transforms/LICM/hoist-mustexec.ll still needs to be updated.
Sat, Apr 27
Fri, Apr 26
Can you please point to patch 5? I think it's missing. (Best directly add it as a parent revision.)
As far as I know we don't have any known issues in funnel shift optimization or codegen. We should check with @spatel though.