- User Since
- May 22 2014, 1:24 PM (295 w, 2 d)
Thanks for cleaning this up!
I don't know enough about jump-threading to approve, but if this is correct, then we should keep the new pass manager in sync with this change?
Adding potential FP reviewers for question of gcc- (potentially-buggy-) compatibility.
This follows the reasoning that we discussed in the earlier discussion, but after re-reading the gcc comment in particular, I'm wondering whether this is what we really want to do...
If FAST_MATH is purely here for compatibility with gcc, then should we mimic gcc behavior in setting that macro even if we think it's buggy?
Ie, when we translate these settings to LLVM's FMF, we can still override the -ffast-math flag by checking the -ffp-contract flag (if I'm seeing it correctly, the existing code will pass that alongside -ffast-math when contract is set to on/off).
Thu, Jan 16
For the new PM question, I assume that's an oversight. I haven't looked too much into the new PM, but I have noticed other possible mistakes there. Eg, it seems like that's running the SpeculativeExecutionPass on all targets?
Wed, Jan 15
Bail out on anything not integer or FP. I'm not sure how to deal with pointers, but added a few basic tests in case we do eventually support those.
I would've taken the test down to 5 or 10 to make it easier to see without scrolling, but this is fine. LGTM.
Tue, Jan 14
I don't know anything about experimental.guard either, but the change LGTM based on similar transforms.
Instcombine's visit* contract is:
// Visitation implementation - Implement instruction combining for different // instruction types. The semantics are as follows: // Return Value: // null - No change was made // I - Change was made, I is still valid, I may be dead though // otherwise - Change was made, replace I with returned instruction
I wish we didn't have such complicated transforms in instcombine, but that's out-of-bounds for this patch. :)
Mon, Jan 13
- Added code comments to better explain the strategy.
- Moved calls lower and within predicate blocks (expect V2 to be undef) where this makes sense to try.
Sun, Jan 12
Fri, Jan 10
This change would also remove the logical gymnastics needed to give special meaning to the mask with respect to undef/poison:
Thu, Jan 9
There's a lot going on here, and the patch doesn't apply cleanly to current source. Can you pre-commit any NFC changes/tests to make this patch smaller? For example, the udiv change/tests are not affected by the mul code?
Wed, Jan 8
I'm very surprised we didn't hit this sooner, but we have this set of 'select' folds in InstCombiner::visitSelectInst() that have existed for at least 10 years, and they are all poison-unsafe:
Thanks all for the reductions!
Made predicate more restrictive and added asserts.
Rebased after pre-committing the cleanup with rG780ba1f22b53.
LGTM - I'll try to push this on behalf of the author shortly.
Tue, Jan 7
Mon, Jan 6
I think this is safe now (although limited to a single basic block), but I don't have enough experience with memop transforms to confidently approve. Please get a 2nd look from @lebedev.ri @jdoerfert @efriedma or someone else.
Sun, Jan 5
Only do the transform if both arms of the select are cmps. Defer canonicalization of single cmp pattern (see TODO comments).