Page MenuHomePhabricator

spatel (Sanjay Patel)
User

Projects

User does not belong to any projects.

User Details

User Since
May 22 2014, 1:24 PM (331 w, 18 h)

Recent Activity

Yesterday

spatel committed rG0a349d5827f6: [SLP] clean up - use 'const' and ArrayRef constructor; NFC (authored by spatel).
[SLP] clean up - use 'const' and ArrayRef constructor; NFC
Thu, Sep 24, 12:32 PM
spatel closed D88074: Add parentheses to eliminate warning.

This got fixed already/identically with: 2c4c659

Thu, Sep 24, 11:08 AM · Restricted Project
spatel committed rGe34bd1e0b03d: [APFloat] prevent NaN morphing into Inf on conversion (PR43907) (authored by spatel).
[APFloat] prevent NaN morphing into Inf on conversion (PR43907)
Thu, Sep 24, 11:02 AM
spatel closed D87835: [APFloat] prevent NaN morphing into Inf on conversion (PR43907).
Thu, Sep 24, 11:02 AM · Restricted Project
spatel added reviewers for D87063: [BitcodeReader] Fix O(N^2) in placeholder replacement algorithm.: dblaikie, fhahn.

Adding more potential reviewers. I'm not sure if anyone is familiar with this code, but others definitely have a better understanding of the data structures than me.

Thu, Sep 24, 9:45 AM · Restricted Project
spatel updated the diff for D87835: [APFloat] prevent NaN morphing into Inf on conversion (PR43907).

Ignore the prior comment/update - I got the review numbers inverted. Sorry about that.

Thu, Sep 24, 9:37 AM · Restricted Project
spatel updated the diff for D88238: [APFloat] convert SNaN to QNaN in convert() and raise Invalid signal.

Updated diff to include clang tests that fail with the setting of the quiet bit.

Thu, Sep 24, 9:36 AM · Restricted Project
spatel updated the diff for D87835: [APFloat] prevent NaN morphing into Inf on conversion (PR43907).

Updated diff to include clang tests that fail with the setting of the quiet bit.

Thu, Sep 24, 9:34 AM · Restricted Project
spatel planned changes to D88238: [APFloat] convert SNaN to QNaN in convert() and raise Invalid signal.

The clang test diffs got dropped from my diff...will update.

Thu, Sep 24, 9:24 AM · Restricted Project
spatel updated the diff for D87835: [APFloat] prevent NaN morphing into Inf on conversion (PR43907).

Patch updated:

  1. Assert that losesInfo was set as expected if we lost the entire NaN payload.
  2. Change code comment from 'TODO' to 'FIXME' based on discussion here.
  3. Added APFloat unittests for better coverage of losesInfo and status.
Thu, Sep 24, 9:20 AM · Restricted Project
spatel requested review of D88238: [APFloat] convert SNaN to QNaN in convert() and raise Invalid signal.
Thu, Sep 24, 9:16 AM · Restricted Project
spatel added a comment to D87835: [APFloat] prevent NaN morphing into Inf on conversion (PR43907).

I think IEEE754's convertFormat operation transforms sNaN->qNaN; I think it makes sense for APFloat to do the same.

Thu, Sep 24, 7:45 AM · Restricted Project
spatel committed rG2625433e77ef: [PhaseOrdering] move test with target requirement to x86 dir (authored by spatel).
[PhaseOrdering] move test with target requirement to x86 dir
Thu, Sep 24, 6:56 AM
spatel committed rG9cf647bb3f88: [PhaseOrdering] move an 'opt' test from x86 codegen; NFC (authored by spatel).
[PhaseOrdering] move an 'opt' test from x86 codegen; NFC
Thu, Sep 24, 6:53 AM
spatel committed rG8e712807e484: [InstCombine] regenerate test checks; NFC (authored by spatel).
[InstCombine] regenerate test checks; NFC
Thu, Sep 24, 6:34 AM
spatel committed rGb2c46633d129: [APFloat] add tests for convert of NAN; NFC (authored by spatel).
[APFloat] add tests for convert of NAN; NFC
Thu, Sep 24, 4:43 AM

Wed, Sep 23

spatel added a comment to D87835: [APFloat] prevent NaN morphing into Inf on conversion (PR43907).

Okay. If we've decided that preserving an sNaN is the best thing to do, as opposed to making it quiet but setting invalid-op, then this looks good to me. But I would be happier if we just made that decision now (what better time?) and then didn't need the TODO.

Wed, Sep 23, 1:23 PM · Restricted Project
spatel committed rG6189a8d9f56a: [TTI] add wrapper for matching vector reduction to reduce code duplication; NFC (authored by spatel).
[TTI] add wrapper for matching vector reduction to reduce code duplication; NFC
Wed, Sep 23, 10:49 AM
spatel added inline comments to D57059: [SLP] Initial support for the vectorization of the non-power-of-2 vectors..
Wed, Sep 23, 7:18 AM · Restricted Project

Tue, Sep 22

spatel accepted D87361: [SelectionDAG] Add helper guard to automatically insert flags.

LGTM - this makes it more likely that we propagate as expected and cleans up the current code.
We probably have more cases like D88063, but that can be handled as follow-up patches.

Tue, Sep 22, 1:04 PM · Restricted Project
spatel accepted D87877: [InstCombine] Fix errno bug in pow expansion to sqrt.

LGTM

Tue, Sep 22, 12:51 PM · Restricted Project
spatel added a reviewer for D88063: [SelectionDAG] Make sure FMF are propagated when getSetcc canonicalizes FP constants to RHS.: qiucf.

What, if any, interaction is there between this and D87361?
Can we solve this with the more general flag propagation scheme proposed there?

Tue, Sep 22, 7:36 AM · Restricted Project
spatel added inline comments to D88066: [InstCombine] For pow(x, +/-0.5), stop falling into pow(x, 1.5), etc. case.
Tue, Sep 22, 6:46 AM · Restricted Project
spatel added inline comments to D57059: [SLP] Initial support for the vectorization of the non-power-of-2 vectors..
Tue, Sep 22, 6:41 AM · Restricted Project
spatel committed rG0c3bfbe4bc21: [SLP] reduce code duplication for checking parent block; NFC (authored by spatel).
[SLP] reduce code duplication for checking parent block; NFC
Tue, Sep 22, 6:27 AM
spatel committed rGbbd49a026692: [SLP] move misplaced code comments; NFC (authored by spatel).
[SLP] move misplaced code comments; NFC
Tue, Sep 22, 6:27 AM
spatel committed rG062276c69109: [SLP] clean up code in gather(); NFC (authored by spatel).
[SLP] clean up code in gather(); NFC
Tue, Sep 22, 6:27 AM
spatel accepted D88074: Add parentheses to eliminate warning.

LGTM - thanks. But please do use clang-format to make the bots happy.

Tue, Sep 22, 6:24 AM · Restricted Project
spatel accepted D88066: [InstCombine] For pow(x, +/-0.5), stop falling into pow(x, 1.5), etc. case.

LGTM

Tue, Sep 22, 6:15 AM · Restricted Project

Mon, Sep 21

spatel committed rG7451bf0b0b6d: [SLP] use std::distance/find to reduce code; NFC (authored by spatel).
[SLP] use std::distance/find to reduce code; NFC
Mon, Sep 21, 1:23 PM
spatel committed rG6bad3caeb079: [InstCombine] use unary shuffle creator to reduce code duplication; NFC (authored by spatel).
[InstCombine] use unary shuffle creator to reduce code duplication; NFC
Mon, Sep 21, 12:35 PM
spatel committed rGbe9350598668: [LoopVectorize] use unary shuffle creator to reduce code duplication; NFC (authored by spatel).
[LoopVectorize] use unary shuffle creator to reduce code duplication; NFC
Mon, Sep 21, 12:35 PM
spatel committed rGa44238cb443f: [SLP] use unary shuffle creator to reduce code duplication; NFC (authored by spatel).
[SLP] use unary shuffle creator to reduce code duplication; NFC
Mon, Sep 21, 10:54 AM
spatel committed rG1e6b240d7d33: [IRBuilder][VectorCombine] make and use a convenience function for unary… (authored by spatel).
[IRBuilder][VectorCombine] make and use a convenience function for unary…
Mon, Sep 21, 10:49 AM
spatel committed rG46075e0b78c3: [SLP] simplify interface for gather(); NFC (authored by spatel).
[SLP] simplify interface for gather(); NFC
Mon, Sep 21, 9:58 AM
spatel added inline comments to D87877: [InstCombine] Fix errno bug in pow expansion to sqrt.
Mon, Sep 21, 8:27 AM · Restricted Project
spatel added inline comments to D87877: [InstCombine] Fix errno bug in pow expansion to sqrt.
Mon, Sep 21, 5:54 AM · Restricted Project

Sun, Sep 20

spatel committed rG7903ae4720a8: [InstCombine] factorize left shifts of add/sub (authored by spatel).
[InstCombine] factorize left shifts of add/sub
Sun, Sep 20, 10:00 AM
spatel committed rGcf75e83275d1: [InstCombine] replace zombie unreachable values with 'undef' before erasing (authored by spatel).
[InstCombine] replace zombie unreachable values with 'undef' before erasing
Sun, Sep 20, 9:25 AM
spatel closed D87965: [InstCombine] replace phi values from unreachable blocks with 'undef'.
Sun, Sep 20, 9:25 AM · Restricted Project
spatel added inline comments to D87965: [InstCombine] replace phi values from unreachable blocks with 'undef'.
Sun, Sep 20, 9:24 AM · Restricted Project
spatel updated the diff for D87965: [InstCombine] replace phi values from unreachable blocks with 'undef'.

Patch updated:

  1. Use the direct approach to fix the bug - replace with undef before trying to delete.
  2. Avoid branch on undef in crashing test case.
Sun, Sep 20, 5:53 AM · Restricted Project

Sat, Sep 19

spatel added a comment to D87149: [InstCombine] erase instructions leading up to unreachable.

Actually, that would be out-of-bounds for instcombine. Removing a phi operand would mean removing the incoming block too, and we don't do that in instcombine. So I think we just bail out if a value still has uses. SimplifyCFG will eventually get it.

Replacing with undef before erasing would do as well.

Sat, Sep 19, 8:30 AM · Restricted Project
spatel requested review of D87965: [InstCombine] replace phi values from unreachable blocks with 'undef'.
Sat, Sep 19, 8:28 AM · Restricted Project
spatel committed rG534e9132afce: [InstCombine] auto-generate test checks; NFC (authored by spatel).
[InstCombine] auto-generate test checks; NFC
Sat, Sep 19, 8:07 AM
spatel committed rG2c3d199fbfaa: [InstCombine] regenerate test checks; NFC (authored by spatel).
[InstCombine] regenerate test checks; NFC
Sat, Sep 19, 7:43 AM
spatel committed rGf74a334fe35b: [ConstantFolding] add undef handling for fmin/fmax intrinsics (authored by spatel).
[ConstantFolding] add undef handling for fmin/fmax intrinsics
Sat, Sep 19, 7:31 AM

Fri, Sep 18

spatel updated the diff for D87835: [APFloat] prevent NaN morphing into Inf on conversion (PR43907).

Patch updated:
Updated code comments; no code or test changes.

Fri, Sep 18, 1:19 PM · Restricted Project
spatel committed rGd3b0644e22a4: [InstSimplify] add tests for constant folding fmin/fmax with undef op; NFC (authored by spatel).
[InstSimplify] add tests for constant folding fmin/fmax with undef op; NFC
Fri, Sep 18, 1:10 PM
spatel added a comment to D87149: [InstCombine] erase instructions leading up to unreachable.

I guess we could try to eliminate phi operands if an incoming block (g.exit) is unreachable? But simplifycfg should do that already, so I don't know if we want instcombine trying to do that too.

Fri, Sep 18, 11:40 AM · Restricted Project
spatel added a comment to D87149: [InstCombine] erase instructions leading up to unreachable.

Unreachable BB's were the obvious potential source for non-empty use-count, but i thought instcombine didn't combine instructions in unreachable blocks.
Maybe that is the bug?

Fri, Sep 18, 10:54 AM · Restricted Project
spatel added a comment to D87149: [InstCombine] erase instructions leading up to unreachable.

Hi!

We've run into a case where instcombine crashes with this patch:

opt -o /dev/null bbi-47401.ll -instcombine

on

@c = external global i16*, align 1

define void @main() {
entry:
  br label %for.cond

for.cond:                                         ; preds = %g.exit, %entry
  %conv1 = phi double [ %conv, %g.exit ], [ undef, %entry ]
  br i1 undef, label %for.end, label %for.body

for.body:                                         ; preds = %for.cond
  %0 = load i16*, i16** @c, align 1
  %1 = load i16, i16* %0, align 1
  %conv = sitofp i16 %1 to double
  unreachable

g.exit:                                           ; No predecessors!
  br label %for.cond

for.end:                                          ; preds = %for.cond
  %conv1.lcssa = phi double [ %conv1, %for.cond ]
  store double %conv1.lcssa, double* undef, align 1
  ret void
}

results in

opt: ../lib/Transforms/InstCombine/InstCombineInternal.h:448: virtual llvm::Instruction *llvm::InstCombinerImpl::eraseInstFromFunction(llvm::Instruction &): Assertion `I.use_empty() && "Cannot erase instruction that is used!"' failed.

I suppose it's the dead block %g.exit that messes up things.

Fri, Sep 18, 10:00 AM · Restricted Project
spatel added a comment to D87188: [InstCombine] Canonicalize SPF to abs intrinc.

I know this has already been reverted but just FYI that I've bisected a ~2% regression in SPEC2017 x264_r on AArch64 to this commit. Presumably this is due to the extra unrolling / cost modelling issue already mentioned?

Fri, Sep 18, 9:09 AM · Restricted Project, Restricted Project
spatel committed rG3f100e64b429: [InstSimplify] fix fmin/fmax miscompile for partial undef vectors (PR47567) (authored by spatel).
[InstSimplify] fix fmin/fmax miscompile for partial undef vectors (PR47567)
Fri, Sep 18, 7:06 AM
spatel committed rG6690de098e43: [InstSimplify] add another test for NaN propagation; NFC (authored by spatel).
[InstSimplify] add another test for NaN propagation; NFC
Fri, Sep 18, 6:20 AM
spatel updated the diff for D87835: [APFloat] prevent NaN morphing into Inf on conversion (PR43907).

Patch updated:

  1. Set the 2nd bit of the significand to preserve NaN-ness and signalling.
  2. Added TODO comments for potentially better/different solutions.
  3. Adjusted tests to provide better coverage of current behavior.
Fri, Sep 18, 5:58 AM · Restricted Project

Thu, Sep 17

spatel added a comment to D87835: [APFloat] prevent NaN morphing into Inf on conversion (PR43907).

So I think the only quibble here is whether it's okay for this to silently produce a qNaN when it might've started with an sNaN. And arguably the current contract of APFloat is to perform operations as if FP exceptions were disabled, and we'll need a slightly different interface if we want to model exceptions properly. So I think this might just be fine as-is.

Thu, Sep 17, 12:33 PM · Restricted Project
spatel added a comment to D86160: [VectorCombine] allow vector loads with mismatched insert type.

I've filed an issue for a performance regression caused by this patch:

https://bugs.llvm.org/show_bug.cgi?id=47558

Thu, Sep 17, 11:32 AM · Restricted Project
spatel committed rG48a23bccf373: [VectorCombine] limit load+insert transform to one-use (authored by spatel).
[VectorCombine] limit load+insert transform to one-use
Thu, Sep 17, 11:29 AM
spatel committed rGddd9575d15ad: [VectorCombine] rearrange bailouts for load insert for efficiency; NFC (authored by spatel).
[VectorCombine] rearrange bailouts for load insert for efficiency; NFC
Thu, Sep 17, 10:51 AM
spatel committed rGe06914b59bf8: [VectorCombine] add test for multi-use load (PR47558); NFC (authored by spatel).
[VectorCombine] add test for multi-use load (PR47558); NFC
Thu, Sep 17, 10:51 AM
spatel committed rGc6ebe3fd002c: [InstSimplify] add tests for FP constant miscompile; NFC (PR43907) (authored by spatel).
[InstSimplify] add tests for FP constant miscompile; NFC (PR43907)
Thu, Sep 17, 9:05 AM
spatel requested review of D87835: [APFloat] prevent NaN morphing into Inf on conversion (PR43907).
Thu, Sep 17, 8:09 AM · Restricted Project
spatel committed rG03783f19dc78: [SLP] sort candidates to increase chance of optimal compare reduction (authored by spatel).
[SLP] sort candidates to increase chance of optimal compare reduction
Thu, Sep 17, 5:50 AM
spatel closed D87772: [SLP] sort candidates to increase chance of optimal compare reduction.
Thu, Sep 17, 5:49 AM · Restricted Project

Wed, Sep 16

spatel accepted D87188: [InstCombine] Canonicalize SPF to abs intrinc.

LGTM - I think we should give this a try as-is (with the one-use check still there), see if anything regresses, then ease/remove the use check as a follow-on.
As noted, we may need to adjust cost models to account for the size/speed difference that's showing up in unrolling/inlining. That's probably because we assume that an intrinsic is expanded to a single instruction vs. the current cmp+sub+select being 3 instructions?

Wed, Sep 16, 1:48 PM · Restricted Project, Restricted Project
spatel updated the diff for D87772: [SLP] sort candidates to increase chance of optimal compare reduction.

Patch updated:
Use stable_sort to reduce movement (not sure how to expose a difference in a test though).

Wed, Sep 16, 1:09 PM · Restricted Project
spatel accepted D87788: [DAGCombiner] Teach visitMLOAD to replace an all ones mask with an unmasked load.

LGTM

Wed, Sep 16, 12:58 PM · Restricted Project
spatel updated the diff for D87772: [SLP] sort candidates to increase chance of optimal compare reduction.

Patch updated:
Added a test to show forming a better reduction than we used to create.

Wed, Sep 16, 10:36 AM · Restricted Project
spatel added inline comments to D87772: [SLP] sort candidates to increase chance of optimal compare reduction.
Wed, Sep 16, 10:35 AM · Restricted Project
spatel committed rGb011611e373c: [SLP] add tests for reduction ordering; NFC (authored by spatel).
[SLP] add tests for reduction ordering; NFC
Wed, Sep 16, 10:28 AM
spatel added inline comments to D87772: [SLP] sort candidates to increase chance of optimal compare reduction.
Wed, Sep 16, 9:35 AM · Restricted Project
spatel requested review of D87772: [SLP] sort candidates to increase chance of optimal compare reduction.
Wed, Sep 16, 8:53 AM · Restricted Project
spatel committed rG24238f09edb9: [SLP] fix formatting; NFC (authored by spatel).
[SLP] fix formatting; NFC
Wed, Sep 16, 5:50 AM
spatel committed rG6a23668e78b0: [SLP] remove uses of 'auto' that obscure functionality; NFC (authored by spatel).
[SLP] remove uses of 'auto' that obscure functionality; NFC
Wed, Sep 16, 5:27 AM
spatel committed rG0cee1bf5d17d: [SLP] remove redundant size check; NFC (authored by spatel).
[SLP] remove redundant size check; NFC
Wed, Sep 16, 5:27 AM
spatel committed rGbbad998bab52: [SLP] move loop index variable declaration to its use; NFC (authored by spatel).
[SLP] move loop index variable declaration to its use; NFC
Wed, Sep 16, 5:00 AM
spatel committed rG158989184e9c: [SLP] change poorly named variable; NFC (authored by spatel).
[SLP] change poorly named variable; NFC
Wed, Sep 16, 4:59 AM

Tue, Sep 15

spatel added a reviewer for D87342: Allow targets to augment computeKnownBits with their analysis using TargetTransformInfo: RKSimon.
Tue, Sep 15, 12:17 PM · Restricted Project
spatel added a comment to D86395: [InstCombine] transform pattern "(~A & B) ^ A -> (A | B)" added.

test53 and test55 has been removed.

We are going in circles. I'll give this 1 more try, and if it still doesn't make sense, then maybe another reviewer can better communicate what we expect for the tests:

  1. There are 4 commutative patterns variations in the code.
  2. Each one of those patterns should have a test (we should have at least 4 tests).
  3. Each test corresponds to exactly one of the commutative patterns.
  4. Each test should be in canonical form without this patch (the baseline CHECK lines should correspond exactly to the IR as written).
  5. The 8 step check I provided earlier may be used to confirm that the tests provide the expected coverage for the code.

@spatel, apologies for the confusion created due to multiple revisions.

Let's consider the initial pattern and associated commutative version of this.

initial pattern:-
(~A & B) ^ A -> (A | B) 

And 8 possible commutative version of this.
1) (~A & B) ^ A -> (A | B) 
2) (~B & A) ^ B -> (A | B)
3) (B & ~A) ^ A -> (A | B)  --> identical to 1)
4) A ^ (~A & B) -> (A | B)  --> identical to 1)
5) A ^ (B & ~A) -> (A | B)  --> identical to 1)
6) (A & ~B) ^ B -> (A | B)  --> identical to 2)
7) B ^ (~B & A) -> (A | B)  --> identical to 2)
8) B ^ (A & ~B) -> (A | B)  --> identical to 2)

As you may notice that all these patterns (IR) are reducing to 2 unique patterns (pattern 1 and pattern 2).
So pattern 1 and 2 are sufficient for the coverage of all commutative variations of the primary pattern.
Does this matches with you understandings ??

Tue, Sep 15, 10:11 AM · Restricted Project
spatel accepted D87538: [VectorCombine] Don't vectorize scalar load under asan/hwasan/memtag/tsan.
Tue, Sep 15, 9:05 AM · Restricted Project
spatel committed rG8985755762a4: [InstSimplify] add limit folds for fmin/fmax (authored by spatel).
[InstSimplify] add limit folds for fmin/fmax
Tue, Sep 15, 8:03 AM
spatel committed rGaa57c1c96707: [InstCombine] fix bug in pow expansion (authored by spatel).
[InstCombine] fix bug in pow expansion
Tue, Sep 15, 6:30 AM
spatel committed rG7ffc9aa538df: [InstCombine] add RUN to show miscompile of pow expansion; NFC (authored by spatel).
[InstCombine] add RUN to show miscompile of pow expansion; NFC
Tue, Sep 15, 6:30 AM
spatel committed rGeb66b04cbecf: [InstCombine] improve test names; NFC (authored by spatel).
[InstCombine] improve test names; NFC
Tue, Sep 15, 6:30 AM
spatel added inline comments to D87538: [VectorCombine] Don't vectorize scalar load under asan/hwasan/memtag/tsan.
Tue, Sep 15, 4:35 AM · Restricted Project

Mon, Sep 14

spatel added inline comments to D87538: [VectorCombine] Don't vectorize scalar load under asan/hwasan/memtag/tsan.
Mon, Sep 14, 1:07 PM · Restricted Project
spatel added a comment to D87538: [VectorCombine] Don't vectorize scalar load under asan/hwasan/memtag/tsan.

I have no experience with the sanitizer requirements, but IIUC we can't accurately sanitize any code that uses this transform with this restriction.
If that's what is happening/intended in other passes, I guess there's no other way around it.
But if it happening in other passes, shouldn't we use some standard function to bail out? For example llvm::mustSuppressSpeculation()?

Mon, Sep 14, 12:09 PM · Restricted Project
spatel committed rG55d371abd7f4: [InstSimplify] add folds for fmin/fmax with 'nnan' (authored by spatel).
[InstSimplify] add folds for fmin/fmax with 'nnan'
Mon, Sep 14, 8:51 AM
spatel added inline comments to D87480: [InstCombine] Simplify select operand based on equality condition.
Mon, Sep 14, 8:41 AM · Restricted Project
spatel committed rG752637616480: [InstSimplify] allow folds for fmin/fmax with 'ninf' (authored by spatel).
[InstSimplify] allow folds for fmin/fmax with 'ninf'
Mon, Sep 14, 8:18 AM
spatel committed rG22c583c3d03a: [InstSimplify] reduce code duplication for fmin/fmax folds; NFC (authored by spatel).
[InstSimplify] reduce code duplication for fmin/fmax folds; NFC
Mon, Sep 14, 7:33 AM
spatel committed rGdae68fdf9ece: [InstSimplify] add/move tests for fmin/fmax; NFC (authored by spatel).
[InstSimplify] add/move tests for fmin/fmax; NFC
Mon, Sep 14, 7:27 AM
spatel committed rG5df9cb5bc71f: [InstSimplify] fix test comments; NFC (authored by spatel).
[InstSimplify] fix test comments; NFC
Mon, Sep 14, 7:08 AM
spatel committed rG7bb9a2f996a3: [InstSimplify] fix miscompiles with maximum/minimum intrinsics (authored by spatel).
[InstSimplify] fix miscompiles with maximum/minimum intrinsics
Mon, Sep 14, 6:10 AM
spatel accepted D87571: [DAGCombiner] Fold fmin/fmax with INF.

Expanded patch still LGTM.
Side note: change "isLargest" name to "isLargestMagnitude" to make that API less confusing?

Mon, Sep 14, 5:40 AM · Restricted Project

Sun, Sep 13

spatel added inline comments to D87571: [DAGCombiner] Fold fmin/fmax with INF.
Sun, Sep 13, 12:52 PM · Restricted Project
spatel accepted D87571: [DAGCombiner] Fold fmin/fmax with INF.

LGTM. Should we common/integrate the caller functions/switch cases? Ie, we're switching on min/max opcode, but then translating that knowledge into the bool flags and then predicating on those flags instead of the opcodes.

Sun, Sep 13, 6:42 AM · Restricted Project
spatel added a comment to D87555: [DivRemPairs] Add an initial case for hoisting to a common predecessor..

I'm not seeing why we need the guaranteed-to-execute checks with the proposed instruction movement. In general, div/rem are special because of their UB problems, but in this case we have:

/// We can largely ignore the normal safety and cost constraints on speculation
/// of these ops when we find a matching pair. This is because we are already
/// guaranteed that any exceptions and most cost are already incurred by the
/// first member of the pair.

In other words, this is similar to hoisting a normal math binop once we find the other member of the pair.

Sun, Sep 13, 6:04 AM · Restricted Project
spatel added a comment to D87555: [DivRemPairs] Add an initial case for hoisting to a common predecessor..

I'm not seeing why we need the guaranteed-to-execute checks with the proposed instruction movement. In general, div/rem are special because of their UB problems, but in this case we have:

Sun, Sep 13, 5:58 AM · Restricted Project