Page MenuHomePhabricator

spatel (Sanjay Patel)
User

Projects

User does not belong to any projects.

User Details

User Since
May 22 2014, 1:24 PM (295 w, 2 d)

Recent Activity

Yesterday

spatel created D72958: [InstSimplify] fold select of vector constants that include undef elements.
Fri, Jan 17, 1:55 PM · Restricted Project
spatel committed rGa8b9c9360111: [InstSimplify] add test for select of vector constants; NFC (authored by spatel).
[InstSimplify] add test for select of vector constants; NFC
Fri, Jan 17, 1:46 PM
spatel committed rG3ae38d95e6c0: [InstSimplify] add test for select of FP constants; NFC (authored by spatel).
[InstSimplify] add test for select of FP constants; NFC
Fri, Jan 17, 1:46 PM
spatel added a comment to D72870: [SelectionDag] Updated FoldConstantArithmetic method signature in preparation for merge with FoldConstantVectorArithmetic.

Thanks for cleaning this up!

The changes all look logically fine to me, but there are numerous formatting problems (80-cols, indentation, etc.).

Try running "clang-format" on the patch?

Fri, Jan 17, 8:55 AM · Restricted Project
spatel added a comment to D72870: [SelectionDag] Updated FoldConstantArithmetic method signature in preparation for merge with FoldConstantVectorArithmetic.

Thanks for cleaning this up!

Fri, Jan 17, 8:15 AM · Restricted Project
spatel committed rG43f60e614a3d: [x86] try harder to form 256-bit unpck* (authored by spatel).
[x86] try harder to form 256-bit unpck*
Fri, Jan 17, 7:46 AM
spatel closed D72575: [x86] try harder to form 256-bit unpck*.
Fri, Jan 17, 7:46 AM · Restricted Project
spatel accepted D72864: [InstCombine] Fix worklist management in return combine.

LGTM

Fri, Jan 17, 6:30 AM · Restricted Project
spatel added reviewers for D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction: andrew.w.kaylor, uweigand, kpn, cameron.mcinally.
Fri, Jan 17, 6:21 AM · Restricted Project
spatel added a comment to D72916: [IPO] Don't run jump threading at Oz.

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?

Fri, Jan 17, 6:20 AM · Restricted Project
spatel added reviewers for D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction: efriedma, scanon, arsenm, echristo, RKSimon.

Adding potential FP reviewers for question of gcc- (potentially-buggy-) compatibility.

Fri, Jan 17, 6:11 AM · Restricted Project
spatel added a comment to D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction.

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).

Fri, Jan 17, 6:01 AM · Restricted Project
spatel committed rGc1e159ef6eb0: [IR] fix Constant::isElementWiseEqual() to allow for all undef elements compare (authored by spatel).
[IR] fix Constant::isElementWiseEqual() to allow for all undef elements compare
Fri, Jan 17, 5:42 AM
spatel committed rGffd3e1607db2: [IR] add unit test for Constant::isElementWiseEqual() for undef corner case; NFC (authored by spatel).
[IR] add unit test for Constant::isElementWiseEqual() for undef corner case; NFC
Fri, Jan 17, 5:32 AM

Thu, Jan 16

spatel committed rG52b44902d059: [IR] fix crash in Constant::isElementWiseEqual() with FP types (authored by spatel).
[IR] fix crash in Constant::isElementWiseEqual() with FP types
Thu, Jan 16, 1:56 PM
spatel closed D72784: [IR] fix crash in Constant::isElementWiseEqual() with FP types.
Thu, Jan 16, 1:56 PM · Restricted Project
spatel added a comment to D72861: [InstCombine] Support disabling expensive combines in opt.
Thu, Jan 16, 1:26 PM · Restricted Project
spatel accepted D72861: [InstCombine] Support disabling expensive combines in opt.

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?
https://github.com/llvm/llvm-project/blob/53b68e676faf208b4a8f817e9bd4ddd522cc6006/llvm/lib/Passes/PassBuilder.cpp#L427

Thu, Jan 16, 12:47 PM · Restricted Project
spatel planned changes to D72643: [InstCombine] form copysign from select of FP constants (PR44153).
Thu, Jan 16, 12:28 PM · Restricted Project
spatel added a comment to D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction.

This all sounds good to me.

So to make sure we're all on the same page, my understanding is that the plan forward is:

  1. Make the Clang change first (including adding another pair of tests for -ffp-contract=on).
  2. Update the LLVM tests illustrating the current baseline state.
  3. Make the LLVM change shown here, and update the tests with the fixes.
  4. Bring in the DAG combiner work that Michael has done internally at Apple.

    Points 1, 2, and 3 are essentially the points in the patch posted here, so I'll do that. And of course Michael will then take on point 4.

    Is that the plan? If yes, I'll transition this item to just be the Clang pieces, and I'll spin off a new one to do the LLVM portion of what is posted here.
Thu, Jan 16, 12:18 PM · Restricted Project
spatel added a comment to D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction.

We crossed that bridge internally at Apple a while ago, meaning I have some code debt for cleaning up open source for fma formation that uses contract and reassoc differently than we do today, both together and separately, case by case.

Thu, Jan 16, 5:37 AM · Restricted Project

Wed, Jan 15

spatel updated the diff for D72784: [IR] fix crash in Constant::isElementWiseEqual() with FP types.

Patch updated:
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.

Wed, Jan 15, 2:43 PM · Restricted Project
spatel added inline comments to D72784: [IR] fix crash in Constant::isElementWiseEqual() with FP types.
Wed, Jan 15, 2:34 PM · Restricted Project
spatel accepted D72807: [InstCombine] Fix worklist management in DSE (PR44552).

I would've taken the test down to 5 or 10 to make it easier to see without scrolling, but this is fine. LGTM.

Wed, Jan 15, 2:06 PM · Restricted Project
spatel added a comment to D72807: [InstCombine] Fix worklist management in DSE (PR44552).

I've added the complete original test case with -instcombine-infinite-loop-threshold=2. It would also be possible to reduce it (it just wouldn't need the full 1000 iterations that trigger the default threshold), but as the test now runs quickly even on a debug build, I figured keeping the original is fine.

Wed, Jan 15, 1:47 PM · Restricted Project
spatel added inline comments to D72675: [Clang][Driver] Fix -ffast-math/-ffp-contract interaction.
Wed, Jan 15, 11:46 AM · Restricted Project
spatel created D72784: [IR] fix crash in Constant::isElementWiseEqual() with FP types.
Wed, Jan 15, 9:44 AM · Restricted Project
spatel committed rG3180af4362be: [InstCombine] reassociate fsub+fsub into fsub+fadd (authored by spatel).
[InstCombine] reassociate fsub+fsub into fsub+fadd
Wed, Jan 15, 8:16 AM
spatel closed D72521: [InstCombine] reassociate fsub+fsub into fsub+fadd.
Wed, Jan 15, 8:16 AM · Restricted Project

Tue, Jan 14

spatel added a comment to D72733: [InstCombine] allow more narrowing of casted select.

I'm not sure what kind of fallout this will cause.
For example, won't this affect 'saturating math' peephole?
Won't this affect clamp (min-max) pattern?

Tue, Jan 14, 2:55 PM · Restricted Project
spatel created D72733: [InstCombine] allow more narrowing of casted select.
Tue, Jan 14, 1:19 PM · Restricted Project
spatel accepted D72558: [InstCombine] Fix worklist management when removing guard intrinsic.

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
Tue, Jan 14, 12:30 PM · Restricted Project
spatel committed rG57cb46851402: [InstCombine] add test for possible cast-of-select transform; NFC (authored by spatel).
[InstCombine] add test for possible cast-of-select transform; NFC
Tue, Jan 14, 11:33 AM
spatel accepted D71164: [InstCombine] Fix infinite loop due to bitcast <-> phi transforms.

I wish we didn't have such complicated transforms in instcombine, but that's out-of-bounds for this patch. :)
LGTM

Tue, Jan 14, 9:18 AM · Restricted Project
spatel committed rGc8a14c2d4773: [IR] fix potential crash in Constant::isElementWiseEqual() (authored by spatel).
[IR] fix potential crash in Constant::isElementWiseEqual()
Tue, Jan 14, 8:59 AM
spatel committed rGcfe2fab708de: [InstSimplify] add tests for vector select; NFC (authored by spatel).
[InstSimplify] add tests for vector select; NFC
Tue, Jan 14, 5:45 AM
spatel added inline comments to D72643: [InstCombine] form copysign from select of FP constants (PR44153).
Tue, Jan 14, 4:58 AM · Restricted Project

Mon, Jan 13

spatel created D72643: [InstCombine] form copysign from select of FP constants (PR44153).
Mon, Jan 13, 1:43 PM · Restricted Project
spatel edited reviewers for D72643: [InstCombine] form copysign from select of FP constants (PR44153), added: nick; removed: kaniandr.
Mon, Jan 13, 1:43 PM · Restricted Project
spatel committed rG80a094e1348a: [InstCombine] add FMF to tests for more coverage; NFC (authored by spatel).
[InstCombine] add FMF to tests for more coverage; NFC
Mon, Jan 13, 1:34 PM
spatel added inline comments to D72575: [x86] try harder to form 256-bit unpck*.
Mon, Jan 13, 1:05 PM · Restricted Project
spatel committed rG69f4cea41399: [InstCombine] add tests for select --> copysign; NFC (authored by spatel).
[InstCombine] add tests for select --> copysign; NFC
Mon, Jan 13, 12:47 PM
spatel updated the diff for D72575: [x86] try harder to form 256-bit unpck*.

Patch updated:

  1. Added code comments to better explain the strategy.
  2. Moved calls lower and within predicate blocks (expect V2 to be undef) where this makes sense to try.
Mon, Jan 13, 11:03 AM · Restricted Project
spatel committed rG26d2ace9e230: [InstSimplify] move tests for select from InstCombine; NFC (authored by spatel).
[InstSimplify] move tests for select from InstCombine; NFC
Mon, Jan 13, 6:16 AM

Sun, Jan 12

spatel created D72575: [x86] try harder to form 256-bit unpck*.
Sun, Jan 12, 11:10 AM · Restricted Project

Fri, Jan 10

spatel created D72521: [InstCombine] reassociate fsub+fsub into fsub+fadd.
Fri, Jan 10, 10:03 AM · Restricted Project
spatel committed rG26cdaeb1f05b: [InstCombine] add tests for fsub; NFC (authored by spatel).
[InstCombine] add tests for fsub; NFC
Fri, Jan 10, 9:07 AM
spatel added a comment to D71568: [InstCombine] `select + mul` -> `select + shl` with power of twos..

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?

Sorry - I didn't see earlier that this patch is part of a sequence.
I'm a bit skeptical about the need to extend the "udiv action" machine. Are the motivating cases really that complicated or could we get away with a simpler pattern match of mul(select...)?

I am not sure but I just reused the code for both division and multiplication -- from my perspective the idea is the same.

Also, I rebased a bit and made the patch smaller as you asked.

So, I basically don't see any reason division machine is doing a lot of things but as abstractions and semantics are mostly the same, I decided to reuse it. And to generalize power 2 multiplication then.

That's true that most of patterns are just one mul(select) but I saw at least one where the depth level was two.

Fri, Jan 10, 8:43 AM · Restricted Project
spatel added a comment to D72467: Remove "mask" operand from shufflevector..

This change would also remove the logical gymnastics needed to give special meaning to the mask with respect to undef/poison:
http://llvm.org/docs/LangRef.html#shufflevector-instruction

Fri, Jan 10, 8:12 AM · Restricted Project, Restricted Project

Thu, Jan 9

spatel committed rG460cbabe170e: [x86] add tests for 2-way splat copy; NFC (authored by spatel).
[x86] add tests for 2-way splat copy; NFC
Thu, Jan 9, 10:14 AM
spatel accepted D71567: [InstCombine] Add tests for `select + mul` for power of twos. NFC.

LGTM

Thu, Jan 9, 9:36 AM · Restricted Project
spatel added a comment to D71568: [InstCombine] `select + mul` -> `select + shl` with power of twos..

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?

Thu, Jan 9, 9:36 AM · Restricted Project
spatel added a comment to D71568: [InstCombine] `select + mul` -> `select + shl` with power of twos..

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?

Thu, Jan 9, 8:49 AM · Restricted Project
spatel committed rG6c04ef472a87: [InstCombine] Z / (1.0 / Y) => (Y * Z) (authored by raghesh).
[InstCombine] Z / (1.0 / Y) => (Y * Z)
Thu, Jan 9, 8:03 AM
spatel closed D72319: [InstCombine] Adding Z / (1.0 / Y) => (Y * Z).
Thu, Jan 9, 8:02 AM · Restricted Project
spatel accepted D72319: [InstCombine] Adding Z / (1.0 / Y) => (Y * Z).

LGTM

Thu, Jan 9, 7:15 AM · Restricted Project
spatel committed rGcb5612e2df89: [DAGCombiner] reduce extract subvector of concat (authored by spatel).
[DAGCombiner] reduce extract subvector of concat
Thu, Jan 9, 6:47 AM
spatel closed D72361: [DAGCombiner] reduce extract subvector of concat.
Thu, Jan 9, 6:46 AM · Restricted Project
spatel committed rGf53b38d12a7b: [InstSimplify] select Cond, true, false --> Cond (authored by spatel).
[InstSimplify] select Cond, true, false --> Cond
Thu, Jan 9, 6:09 AM
spatel closed D72412: [InstSimplify] select Cond, true, false --> Cond.
Thu, Jan 9, 6:09 AM · Restricted Project
spatel committed rG032a9393a739: [InstCombine] Use minimal FMF in testcase for Z / (1.0 / Y) => (Y * Z); NFC (authored by spatel).
[InstCombine] Use minimal FMF in testcase for Z / (1.0 / Y) => (Y * Z); NFC
Thu, Jan 9, 5:22 AM
spatel closed D72431: [InstCombine] Minor update to testcase for Z / (1.0 / Y) => (Y * Z).
Thu, Jan 9, 5:22 AM · Restricted Project
spatel accepted D72431: [InstCombine] Minor update to testcase for Z / (1.0 / Y) => (Y * Z).

LGTM

Thu, Jan 9, 5:21 AM · Restricted Project

Wed, Jan 8

spatel created D72412: [InstSimplify] select Cond, true, false --> Cond.
Wed, Jan 8, 1:37 PM · Restricted Project
spatel committed rG0b8ce37d6474: [InstSimplify] add tests for select of true/false; NFC (authored by spatel).
[InstSimplify] add tests for select of true/false; NFC
Wed, Jan 8, 1:28 PM
spatel added a comment to D72361: [DAGCombiner] reduce extract subvector of concat.

Patch updated:
Made predicate more restrictive and added asserts.

Can a test case be constructed for that?

(That case could be implemented as a shuffle, if we don't do that already)

Wed, Jan 8, 11:55 AM · Restricted Project
spatel committed rG31992a69b808: [x86] add test for concat-extract corner case; NFC (authored by spatel).
[x86] add test for concat-extract corner case; NFC
Wed, Jan 8, 11:45 AM
spatel added a comment to D72396: [InstCombine] fold zext of masked bit set/clear.

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:

Wed, Jan 8, 10:40 AM · Restricted Project
spatel added a comment to D72396: [InstCombine] fold zext of masked bit set/clear.

Thanks all for the reductions!

Wed, Jan 8, 10:11 AM · Restricted Project
spatel updated the diff for D72361: [DAGCombiner] reduce extract subvector of concat.

Patch updated:
Made predicate more restrictive and added asserts.

Wed, Jan 8, 8:56 AM · Restricted Project
spatel added inline comments to D72361: [DAGCombiner] reduce extract subvector of concat.
Wed, Jan 8, 8:37 AM · Restricted Project
spatel added inline comments to D72361: [DAGCombiner] reduce extract subvector of concat.
Wed, Jan 8, 8:09 AM · Restricted Project
spatel added inline comments to D72388: [InstCombine] Adding testcase for Z / (1.0 / Y) => (Y * Z).
Wed, Jan 8, 7:50 AM · Restricted Project
spatel added inline comments to D72319: [InstCombine] Adding Z / (1.0 / Y) => (Y * Z).
Wed, Jan 8, 7:50 AM · Restricted Project
spatel committed rG5dfd52398f5c: [InstCombine] Adding testcase for Z / (1.0 / Y) => (Y * Z); NFC (authored by spatel).
[InstCombine] Adding testcase for Z / (1.0 / Y) => (Y * Z); NFC
Wed, Jan 8, 7:41 AM
spatel closed D72388: [InstCombine] Adding testcase for Z / (1.0 / Y) => (Y * Z).
Wed, Jan 8, 7:40 AM · Restricted Project
spatel updated the diff for D72361: [DAGCombiner] reduce extract subvector of concat.

Patch updated:
Rebased after pre-committing the cleanup with rG780ba1f22b53.

Wed, Jan 8, 7:31 AM · Restricted Project
spatel committed rG780ba1f22b53: [DAGCombiner] clean up extract-of-concat fold; NFC (authored by spatel).
[DAGCombiner] clean up extract-of-concat fold; NFC
Wed, Jan 8, 7:22 AM
spatel accepted D72388: [InstCombine] Adding testcase for Z / (1.0 / Y) => (Y * Z).

LGTM - I'll try to push this on behalf of the author shortly.

Wed, Jan 8, 7:12 AM · Restricted Project
spatel added a comment to D72361: [DAGCombiner] reduce extract subvector of concat.

LGTM - some of NFC refactoring as pre-commits would make sense to separate it from the new features.

Wed, Jan 8, 6:34 AM · Restricted Project
spatel updated the summary of D72396: [InstCombine] fold zext of masked bit set/clear.
Wed, Jan 8, 6:25 AM · Restricted Project
spatel created D72396: [InstCombine] fold zext of masked bit set/clear.
Wed, Jan 8, 6:25 AM · Restricted Project

Tue, Jan 7

spatel created D72361: [DAGCombiner] reduce extract subvector of concat.
Tue, Jan 7, 2:06 PM · Restricted Project
spatel committed rG6d52edebc99a: [x86] add tests for extract-of-concat; NFC (authored by spatel).
[x86] add tests for extract-of-concat; NFC
Tue, Jan 7, 12:49 PM
spatel committed rGf8962571f70a: [InstCombine] try to pull 'not' of select into compare operands (authored by spatel).
[InstCombine] try to pull 'not' of select into compare operands
Tue, Jan 7, 7:52 AM
spatel closed D72007: [InstCombine] try to pull 'not' of select into compare operands.
Tue, Jan 7, 7:52 AM · Restricted Project
spatel added inline comments to D72319: [InstCombine] Adding Z / (1.0 / Y) => (Y * Z).
Tue, Jan 7, 7:24 AM · Restricted Project
spatel committed rG58e2e92a57fc: [DAGCombiner] reduce shuffle of concat of same vector (authored by spatel).
[DAGCombiner] reduce shuffle of concat of same vector
Tue, Jan 7, 6:56 AM
spatel closed D72300: [DAGCombiner] reduce shuffle of concat of same vector.
Tue, Jan 7, 6:56 AM · Restricted Project
spatel added a comment to D72300: [DAGCombiner] reduce shuffle of concat of same vector.

Its fine to leave it where it is - LGTM cheers.

Tue, Jan 7, 6:27 AM · Restricted Project
spatel added a comment to D72300: [DAGCombiner] reduce shuffle of concat of same vector.

Why doesn't partitionShuffleOfConcats catch this?

Tue, Jan 7, 5:41 AM · Restricted Project

Mon, Jan 6

spatel created D72300: [DAGCombiner] reduce shuffle of concat of same vector.
Mon, Jan 6, 1:01 PM · Restricted Project
spatel committed rG22cec48dacc6: [x86] add tests for concat self + shuffle; NFC (authored by spatel).
[x86] add tests for concat self + shuffle; NFC
Mon, Jan 6, 12:33 PM
spatel added a comment to D71828: [InstCombine] Convert vector store to scalar store if only one element updated.

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.

Mon, Jan 6, 9:02 AM · Restricted Project

Sun, Jan 5

spatel updated the diff for D72007: [InstCombine] try to pull 'not' of select into compare operands.

Patch updated:
Only do the transform if both arms of the select are cmps. Defer canonicalization of single cmp pattern (see TODO comments).

Sun, Jan 5, 9:17 AM · Restricted Project

Sat, Jan 4

spatel added a comment to D72007: [InstCombine] try to pull 'not' of select into compare operands.

Looks good to me in principle, the case where we get rid of not is good,
but i'm not sure about the case where we only manage to sink it into one of the hands.
To me that seems to be the opposite of the current general canonicalizaion we do.

Sat, Jan 4, 6:18 AM · Restricted Project

Fri, Jan 3

spatel committed rGca7fdd41bda0: [DAGCombiner] fix miscompile in translating (X & undef) to shuffle (authored by spatel).
[DAGCombiner] fix miscompile in translating (X & undef) to shuffle
Fri, Jan 3, 12:02 PM
spatel committed rG32ccafd0f253: [x86] add test for miscompile in XformToShuffleWithZero(); NFC (authored by spatel).
[x86] add test for miscompile in XformToShuffleWithZero(); NFC
Fri, Jan 3, 11:51 AM
spatel committed rG164058274364: [InstCombine] replace undef elements in vector constant when doing icmp folds… (authored by spatel).
[InstCombine] replace undef elements in vector constant when doing icmp folds…
Fri, Jan 3, 6:18 AM
spatel closed D72101: [InstCombine] replace undef elements in vector constant when doing icmp folds (PR44383).
Fri, Jan 3, 6:17 AM · Restricted Project