Page MenuHomePhabricator

spatel (Sanjay Patel)
User

Projects

User does not belong to any projects.

User Details

User Since
May 22 2014, 1:24 PM (264 w, 4 d)

Recent Activity

Today

spatel added a comment to D59909: [X86] Add custom isel to select ADD/SUB/OR/XOR/AND to their non-immediate forms under optsize when the immediate has additional users..

@spatel @andreadb Any comments?

Mon, Jun 17, 12:31 PM · Restricted Project
spatel added a comment to D63390: [Codegen] TargetLowering::SimplifySetCC(): omit urem when possible.

Thank you for the reviews.

LGTM. I have no idea if it's worth the trade-off, but we could do this sooner in IR (instcombine) instead of or in addition to SDAG?

Indeed, this fold is missed in middle-end too, but the regression at hand is in back-end test,
so i'm not sure if we should just hand-wave and only fix it in middle-end.
Also, where should the middle-end fix be? Again InstCombine?

Mon, Jun 17, 11:36 AM · Restricted Project
spatel accepted D63405: GlobalISel: Don't lose fneg flags when lowering to fsub.

This matches my mental model for FMF propagation, so LGTM.

Mon, Jun 17, 11:23 AM
spatel accepted D63390: [Codegen] TargetLowering::SimplifySetCC(): omit urem when possible.

LGTM. I have no idea if it's worth the trade-off, but we could do this sooner in IR (instcombine) instead of or in addition to SDAG?

Mon, Jun 17, 11:06 AM · Restricted Project
spatel added inline comments to D63391: [CodeGen] [SelectionDAG] More efficient code for X % C == 0 (UREM case) (try 2).
Mon, Jun 17, 10:11 AM · Restricted Project
spatel added a comment to D63405: GlobalISel: Don't lose fneg flags when lowering to fsub.

I'm not familiar with how flags are being used in this layer, but union of flags doesn't match the way we deal with FMF in IR. If the fneg is strict (disregard the irrelevant 'arcp' in that last test), then it's invalid to assume that it inherits the 'nsz' property from its operand.

I would expect the lowering here to just preserve the flag on the original instruction, and not propagate the source (as was done here previously). The corresponding expansion in the DAG has a TODO to preserve the flags, so there isn't precedent for this

Let me know if I'm not seeing this correctly (haven't looked at MIR very much - it might help if the test was committed with the baseline output first, so we just have a diff in this patch).
Did we start with 'fneg arcp X' and end up with 'fsub arcp nsz -0.0, X'? Preserving (rather than unioning) the flag should end up with only 'fsub arcp -0.0, X'?

The pre-patch code would fold fneg (fadd flags x, y) -> fsub flags -0.0, (fadd flags x, y), and entirely ignore any flags on fneg. I'm not entirely sure this is correct on its own

Mon, Jun 17, 9:53 AM
spatel added a comment to D63405: GlobalISel: Don't lose fneg flags when lowering to fsub.

I'm not familiar with how flags are being used in this layer, but union of flags doesn't match the way we deal with FMF in IR. If the fneg is strict (disregard the irrelevant 'arcp' in that last test), then it's invalid to assume that it inherits the 'nsz' property from its operand.

I would expect the lowering here to just preserve the flag on the original instruction, and not propagate the source (as was done here previously). The corresponding expansion in the DAG has a TODO to preserve the flags, so there isn't precedent for this

Mon, Jun 17, 9:36 AM
spatel added reviewers for D63405: GlobalISel: Don't lose fneg flags when lowering to fsub: hfinkel, efriedma.

I'm not familiar with how flags are being used in this layer, but union of flags doesn't match the way we deal with FMF in IR. If the fneg is strict (disregard the irrelevant 'arcp' in that last test), then it's invalid to assume that it inherits the 'nsz' property from its operand.

Mon, Jun 17, 9:11 AM

Yesterday

spatel committed rGc8d88ad1a916: [CodeGenPrepare][x86] shift both sides of a vector select when profitable (authored by spatel).
[CodeGenPrepare][x86] shift both sides of a vector select when profitable
Sun, Jun 16, 8:27 AM
spatel committed rL363511: [CodeGenPrepare][x86] shift both sides of a vector select when profitable.
[CodeGenPrepare][x86] shift both sides of a vector select when profitable
Sun, Jun 16, 8:26 AM
spatel closed D63233: [CodeGenPrepare] shift both sides of a vector select when profitable.
Sun, Jun 16, 8:26 AM · Restricted Project
spatel committed rGd14389c0a550: [x86] split 256-bit vector selects if operands are vector concats (authored by spatel).
[x86] split 256-bit vector selects if operands are vector concats
Sun, Jun 16, 7:04 AM
spatel committed rL363508: [x86] split 256-bit vector selects if operands are vector concats.
[x86] split 256-bit vector selects if operands are vector concats
Sun, Jun 16, 7:04 AM
spatel closed D63364: [x86] split 256-bit vector selects if operands are vector concats.
Sun, Jun 16, 7:04 AM · Restricted Project

Sat, Jun 15

spatel added a comment to D63364: [x86] split 256-bit vector selects if operands are vector concats.

Looks ok.
Is there some costmodel here, or do we always (well, when we see concatenation, we don't seem to introduce it
intentionally) want to do this, in the hope that two smaller ops are always at least as good as one wider op?

Sat, Jun 15, 7:14 AM · Restricted Project

Fri, Jun 14

spatel created D63364: [x86] split 256-bit vector selects if operands are vector concats.
Fri, Jun 14, 3:05 PM · Restricted Project
spatel committed rG501bb982b935: [x86] add test for 256-bit blendv with AVX targets; NFC (authored by spatel).
[x86] add test for 256-bit blendv with AVX targets; NFC
Fri, Jun 14, 1:01 PM
spatel committed rL363448: [x86] add test for 256-bit blendv with AVX targets; NFC.
[x86] add test for 256-bit blendv with AVX targets; NFC
Fri, Jun 14, 1:00 PM
spatel updated the diff for D63233: [CodeGenPrepare] shift both sides of a vector select when profitable.

Patch updated with test changes:

  1. Added a bigger test for the original PR37428 example because that shows a difference: with AVX1, we do not split the ymm ops any more. I'm not sure if that's better or worse than having mul/sitofp ops in the loop, but since the original request is for an SSE target, I think we can deal with that independently.
  2. Added an "enable-debugify" RUN to the IR test file to verify that we are not creating naked instructions. The IRBuilder takes care of this for us here, but I fixed a related problem in rL363409.
Fri, Jun 14, 9:12 AM · Restricted Project
spatel committed rG75312aa805c3: [x86] move vector shift tests for PR37428; NFC (authored by spatel).
[x86] move vector shift tests for PR37428; NFC
Fri, Jun 14, 8:23 AM
spatel committed rL363411: [x86] move vector shift tests for PR37428; NFC.
[x86] move vector shift tests for PR37428; NFC
Fri, Jun 14, 8:20 AM
spatel committed rG7ea378b940b4: [CodeGenPrepare] propagate debuginfo when copying a shuffle (authored by spatel).
[CodeGenPrepare] propagate debuginfo when copying a shuffle
Fri, Jun 14, 8:03 AM
spatel committed rL363409: [CodeGenPrepare] propagate debuginfo when copying a shuffle.
[CodeGenPrepare] propagate debuginfo when copying a shuffle
Fri, Jun 14, 8:03 AM
spatel planned changes to D63233: [CodeGenPrepare] shift both sides of a vector select when profitable.

For reference, the vector shift TLI hook was originally added for a related CGP transform:
rL201655

Fri, Jun 14, 7:21 AM · Restricted Project
spatel committed rGe5a78cd90f2b: [x86] add test for original example in PR37428; NFC (authored by spatel).
[x86] add test for original example in PR37428; NFC
Fri, Jun 14, 6:42 AM
spatel committed rL363392: [x86] add test for original example in PR37428; NFC.
[x86] add test for original example in PR37428; NFC
Fri, Jun 14, 6:41 AM

Thu, Jun 13

spatel added a child revision for D63294: [Analysis] enhance FP library function prototype checking to match types with name suffix : D63214: [InstCombine] canonicalize fmin/fmax to LLVM intrinsics minnum/maxnum.
Thu, Jun 13, 12:18 PM · Restricted Project
spatel added a parent revision for D63214: [InstCombine] canonicalize fmin/fmax to LLVM intrinsics minnum/maxnum: D63294: [Analysis] enhance FP library function prototype checking to match types with name suffix .
Thu, Jun 13, 12:18 PM · Restricted Project
spatel created D63294: [Analysis] enhance FP library function prototype checking to match types with name suffix .
Thu, Jun 13, 12:13 PM · Restricted Project
spatel committed rG5bf7f81aa8cc: [InstCombine] add test for failed libfunction prototype matching; NFC (authored by spatel).
[InstCombine] add test for failed libfunction prototype matching; NFC
Thu, Jun 13, 11:27 AM
spatel committed rL363291: [InstCombine] add test for failed libfunction prototype matching; NFC.
[InstCombine] add test for failed libfunction prototype matching; NFC
Thu, Jun 13, 11:26 AM
spatel committed rG4d93fb528ec0: [InstCombine] auto-generate complete test checks; NFC (authored by spatel).
[InstCombine] auto-generate complete test checks; NFC
Thu, Jun 13, 11:12 AM
spatel committed rL363286: [InstCombine] auto-generate complete test checks; NFC.
[InstCombine] auto-generate complete test checks; NFC
Thu, Jun 13, 11:11 AM

Wed, Jun 12

spatel created D63233: [CodeGenPrepare] shift both sides of a vector select when profitable.
Wed, Jun 12, 4:30 PM · Restricted Project
spatel committed rGa1421e834714: [x86] add tests for vector shifts; NFC (authored by spatel).
[x86] add tests for vector shifts; NFC
Wed, Jun 12, 2:30 PM
spatel committed rL363203: [x86] add tests for vector shifts; NFC.
[x86] add tests for vector shifts; NFC
Wed, Jun 12, 2:27 PM
spatel created D63214: [InstCombine] canonicalize fmin/fmax to LLVM intrinsics minnum/maxnum.
Wed, Jun 12, 9:02 AM · Restricted Project
spatel committed rG64006896ac0b: [InstCombine] add tests for fmin/fmax libcalls; NFC (authored by spatel).
[InstCombine] add tests for fmin/fmax libcalls; NFC
Wed, Jun 12, 8:27 AM
spatel committed rL363175: [InstCombine] add tests for fmin/fmax libcalls; NFC.
[InstCombine] add tests for fmin/fmax libcalls; NFC
Wed, Jun 12, 8:26 AM
spatel committed rG082a41994ac9: [InstCombine] add tests for fcmp+select with FMF (minnum/maxnum); NFC (authored by spatel).
[InstCombine] add tests for fcmp+select with FMF (minnum/maxnum); NFC
Wed, Jun 12, 6:49 AM
spatel committed rL363163: [InstCombine] add tests for fcmp+select with FMF (minnum/maxnum); NFC.
[InstCombine] add tests for fcmp+select with FMF (minnum/maxnum); NFC
Wed, Jun 12, 6:48 AM

Tue, Jun 11

spatel committed rG40e3bdf8764d: [Analysis] add isSplatValue() for vectors in IR (authored by spatel).
[Analysis] add isSplatValue() for vectors in IR
Tue, Jun 11, 3:25 PM
spatel committed rL363106: [Analysis] add isSplatValue() for vectors in IR.
[Analysis] add isSplatValue() for vectors in IR
Tue, Jun 11, 3:24 PM
spatel closed D63138: [Analysis] add isSplatValue() for vectors in IR.
Tue, Jun 11, 3:24 PM · Restricted Project
spatel added inline comments to D63138: [Analysis] add isSplatValue() for vectors in IR.
Tue, Jun 11, 11:52 AM · Restricted Project
spatel accepted D62644: [EarlyCSE] Ensure equal keys have the same hash value.

LGTM - but let's wait a ~day to commit in case anyone else has comments.

Tue, Jun 11, 11:00 AM · Restricted Project
spatel added inline comments to D62644: [EarlyCSE] Ensure equal keys have the same hash value.
Tue, Jun 11, 10:32 AM · Restricted Project
spatel added a comment to D63138: [Analysis] add isSplatValue() for vectors in IR.

We have 2 text comment acknowledgements ("Seems fine", "Looks ok") - thanks for the prompt reviews!...can someone make it official by toggling the Phab state to 'Approved'? :)

Tue, Jun 11, 10:03 AM · Restricted Project
spatel added inline comments to D62414: [InstCombine] canonicalize fcmp+select to minnum/maxnum intrinsics.
Tue, Jun 11, 9:23 AM · Restricted Project
spatel added inline comments to D62414: [InstCombine] canonicalize fcmp+select to minnum/maxnum intrinsics.
Tue, Jun 11, 8:55 AM · Restricted Project
spatel added inline comments to D62414: [InstCombine] canonicalize fcmp+select to minnum/maxnum intrinsics.
Tue, Jun 11, 8:15 AM · Restricted Project
spatel updated the diff for D63138: [Analysis] add isSplatValue() for vectors in IR.

Patch updated:

  1. Add text to header comment to indicate that this may be more powerful than getSplatValue().
  2. Add TODO comment about recursing through several other types of operands.
Tue, Jun 11, 8:01 AM · Restricted Project
spatel created D63138: [Analysis] add isSplatValue() for vectors in IR.
Tue, Jun 11, 7:09 AM · Restricted Project
spatel added reviewers for D62414: [InstCombine] canonicalize fcmp+select to minnum/maxnum intrinsics: nikic, cameron.mcinally, craig.topper.

Ping * 2.

Tue, Jun 11, 6:48 AM · Restricted Project

Mon, Jun 10

spatel accepted D62612: [InstCombine] Handle -(X-Y) --> (Y-X) for unary fneg when NSZ.

This matches the existing fold with 2 fsubs, and I agree with the comment in the description regarding IEEE-754 - fsub doesn't need to preserve the sign bit of a NaN. LGTM.

Mon, Jun 10, 2:49 PM · Restricted Project
spatel accepted D62629: [InstCombine] Update fptrunc (fneg x)) -> (fneg (fptrunc x) for unary FNeg.

LGTM

Mon, Jun 10, 2:38 PM · Restricted Project
spatel committed rGb0f98d34225d: [Analysis] add unit test file for VectorUtils; NFC (authored by spatel).
[Analysis] add unit test file for VectorUtils; NFC
Mon, Jun 10, 11:20 AM
spatel committed rL362972: [Analysis] add unit test file for VectorUtils; NFC.
[Analysis] add unit test file for VectorUtils; NFC
Mon, Jun 10, 11:19 AM
spatel added a comment to D62629: [InstCombine] Update fptrunc (fneg x)) -> (fneg (fptrunc x) for unary FNeg.

Check hasOneUse() before performing combine.

Mon, Jun 10, 9:59 AM · Restricted Project
spatel added a comment to D63038: [SimplifyLibCalls] powf(x, sitofp(n)) -> powi(x, n).

Can you please provide some more information under which circumstances powf(x, (float) y) will provide a different result than powi(x, y), and which fast-math flags specifically are necessary to make that transform legal?

I don’t have such info, I personally think this is fine also without fast math, but since I cannot say it for sure, In the first version, I put it under fast math.

Maybe some experts like @spatel or @efriedma can confirm that this transformation is always fine?

Mon, Jun 10, 9:30 AM · Restricted Project
spatel committed rG9650c95b7e55: [InstCombine] allow unordered preds when canonicalizing to fabs() (authored by spatel).
[InstCombine] allow unordered preds when canonicalizing to fabs()
Mon, Jun 10, 8:37 AM
spatel committed rL362954: [InstCombine] allow unordered preds when canonicalizing to fabs().
[InstCombine] allow unordered preds when canonicalizing to fabs()
Mon, Jun 10, 8:36 AM
spatel committed rG07bba688895d: [InstCombine] add tests for fabs() with unordered preds; NFC (authored by spatel).
[InstCombine] add tests for fabs() with unordered preds; NFC
Mon, Jun 10, 8:06 AM
spatel committed rL362949: [InstCombine] add tests for fabs() with unordered preds; NFC.
[InstCombine] add tests for fabs() with unordered preds; NFC
Mon, Jun 10, 8:05 AM
spatel committed rG85de9634e641: [InstCombine] fix bug in canonicalization to fabs() (authored by spatel).
[InstCombine] fix bug in canonicalization to fabs()
Mon, Jun 10, 7:57 AM
spatel committed rL362945: [InstCombine] fix bug in canonicalization to fabs().
[InstCombine] fix bug in canonicalization to fabs()
Mon, Jun 10, 7:54 AM
spatel committed rG8b6d9f60ed71: [InstCombine] change canonicalization to fabs() to use FMF on fsub (authored by spatel).
[InstCombine] change canonicalization to fabs() to use FMF on fsub
Mon, Jun 10, 7:47 AM
spatel committed rL362943: [InstCombine] change canonicalization to fabs() to use FMF on fsub.
[InstCombine] change canonicalization to fabs() to use FMF on fsub
Mon, Jun 10, 7:44 AM
spatel committed rG8cd8c5784b81: [InstCombine] allow unordered preds when canonicalizing to fabs() (authored by spatel).
[InstCombine] allow unordered preds when canonicalizing to fabs()
Mon, Jun 10, 7:14 AM
spatel committed rL362937: [InstCombine] allow unordered preds when canonicalizing to fabs().
[InstCombine] allow unordered preds when canonicalizing to fabs()
Mon, Jun 10, 7:14 AM
spatel committed rG4cdd3ceb572d: [InstCombine] add tests for fcmp unordered pred -> fabs (PR42179); NFC (authored by spatel).
[InstCombine] add tests for fcmp unordered pred -> fabs (PR42179); NFC
Mon, Jun 10, 7:04 AM
spatel committed rL362936: [InstCombine] add tests for fcmp unordered pred -> fabs (PR42179); NFC.
[InstCombine] add tests for fcmp unordered pred -> fabs (PR42179); NFC
Mon, Jun 10, 7:04 AM

Sun, Jun 9

spatel accepted D63004: [TargetLowering] Simplify (ctpop x) == 1.

LGTM

Sun, Jun 9, 9:58 AM · Restricted Project
spatel committed rG87cd16a86efb: [InstCombine] change canonicalization to fabs() to use FMF on fneg (authored by spatel).
[InstCombine] change canonicalization to fabs() to use FMF on fneg
Sun, Jun 9, 9:20 AM
spatel committed rL362909: [InstCombine] change canonicalization to fabs() to use FMF on fneg.
[InstCombine] change canonicalization to fabs() to use FMF on fneg
Sun, Jun 9, 9:20 AM
spatel accepted D62521: [IRBuilder] Add CreateFNegFMF(...) to the IRBuilder.

LGTM

Sun, Jun 9, 8:46 AM · Restricted Project
spatel added a comment to D63004: [TargetLowering] Simplify (ctpop x) == 1.

@spatel Thanks for review

What about current revision? Is it OK for you?

Sun, Jun 9, 8:41 AM · Restricted Project
spatel added a comment to D63004: [TargetLowering] Simplify (ctpop x) == 1.

@spatel Maybe we should just expand in it instcombine? I found something interesting...

Sun, Jun 9, 8:02 AM · Restricted Project
spatel added a comment to D62713: [MIR-Canon] Hardening propagateLocalCopies..

I'm seeing intermittent failures locally for this test on macOS since this was committed, and that seems confirmed by this bot:
http://green.lab.llvm.org/green/blue/organizations/jenkins/clang-stage1-cmake-RA-incremental/activity
(click 'Show More' at the bottom to extend the history and see this test as the only toggling failure with seemingly random NFC changes)

Sun, Jun 9, 7:43 AM · Restricted Project
spatel added a comment to D62713: [MIR-Canon] Hardening propagateLocalCopies..

I'm seeing intermittent failures locally for this test on macOS since this was committed, and that seems confirmed by this bot:
http://green.lab.llvm.org/green/blue/organizations/jenkins/clang-stage1-cmake-RA-incremental/activity
(click 'Show More' at the bottom to extend the history and see this test as the only toggling failure with seemingly random NFC changes)

Sun, Jun 9, 7:18 AM · Restricted Project
spatel committed rG866db1022845: [InstSimplify] reduce code duplication for fcmp folds; NFC (authored by spatel).
[InstSimplify] reduce code duplication for fcmp folds; NFC
Sun, Jun 9, 6:58 AM
spatel committed rL362904: [InstSimplify] reduce code duplication for fcmp folds; NFC.
[InstSimplify] reduce code duplication for fcmp folds; NFC
Sun, Jun 9, 6:55 AM
spatel committed rG73f5a855b333: [InstSimplify] enhance fcmp fold with never-nan operand (authored by spatel).
[InstSimplify] enhance fcmp fold with never-nan operand
Sun, Jun 9, 6:47 AM
spatel committed rL362903: [InstSimplify] enhance fcmp fold with never-nan operand.
[InstSimplify] enhance fcmp fold with never-nan operand
Sun, Jun 9, 6:45 AM
spatel committed rGde4d4d5049e5: [InstSimplify] add tests for fcmp with known-never-nan operands; NFC (authored by spatel).
[InstSimplify] add tests for fcmp with known-never-nan operands; NFC
Sun, Jun 9, 6:28 AM
spatel committed rL362902: [InstSimplify] add tests for fcmp with known-never-nan operands; NFC.
[InstSimplify] add tests for fcmp with known-never-nan operands; NFC
Sun, Jun 9, 6:27 AM

Sat, Jun 8

spatel committed rG4329c15f1175: [InstSimplify] enhance fcmp fold with never-nan operand (authored by spatel).
[InstSimplify] enhance fcmp fold with never-nan operand
Sat, Jun 8, 8:12 AM
spatel committed rL362879: [InstSimplify] enhance fcmp fold with never-nan operand.
[InstSimplify] enhance fcmp fold with never-nan operand
Sat, Jun 8, 8:12 AM
spatel closed D62979: [InstSimplify] enhance/fix fcmp fold with never-nan operand.
Sat, Jun 8, 8:12 AM · Restricted Project

Fri, Jun 7

spatel added inline comments to D62705: [IR] Add UnaryOperator::CreateFNegFMF(...).
Fri, Jun 7, 11:38 AM · Restricted Project
spatel added inline comments to D62705: [IR] Add UnaryOperator::CreateFNegFMF(...).
Fri, Jun 7, 9:52 AM · Restricted Project
spatel accepted D62705: [IR] Add UnaryOperator::CreateFNegFMF(...).

LGTM

Fri, Jun 7, 9:43 AM · Restricted Project
spatel updated the diff for D62979: [InstSimplify] enhance/fix fcmp fold with never-nan operand.

Patch updated:
Only add a check for isKnownNeverNaN(). We can remove FMF.noNaNs() independently when that's less likely to introduce regressions.

Fri, Jun 7, 9:41 AM · Restricted Project
spatel added inline comments to D62979: [InstSimplify] enhance/fix fcmp fold with never-nan operand.
Fri, Jun 7, 9:26 AM · Restricted Project
spatel committed rGe490e4a0e7ea: [Analysis] simplify code for getSplatValue(); NFC (authored by spatel).
[Analysis] simplify code for getSplatValue(); NFC
Fri, Jun 7, 9:09 AM
spatel committed rL362810: [Analysis] simplify code for getSplatValue(); NFC.
[Analysis] simplify code for getSplatValue(); NFC
Fri, Jun 7, 9:09 AM
spatel added a reviewer for D63004: [TargetLowering] Simplify (ctpop x) == 1: RKSimon.

How does the output differ from the default expansion/optimization of ctpop? Please commit the tests with baseline CHECK lines, so we just show that diff. Add tests for another target too (AArch64?).

Fri, Jun 7, 6:37 AM · Restricted Project
spatel committed rG6880bceda2df: [x86] narrow extract subvector of vector select (authored by spatel).
[x86] narrow extract subvector of vector select
Fri, Jun 7, 6:18 AM
spatel committed rL362797: [x86] narrow extract subvector of vector select.
[x86] narrow extract subvector of vector select
Fri, Jun 7, 6:18 AM
spatel closed D62969: [x86] narrow extract subvector of vector select.
Fri, Jun 7, 6:18 AM · Restricted Project