Page MenuHomePhabricator

eli.friedman (Eli Friedman)
User

Projects

User does not belong to any projects.

User Details

User Since
Aug 24 2012, 2:47 PM (328 w, 5 d)

Recent Activity

Aug 10 2016

eli.friedman added a comment to D21007: DSE: Don't remove stores made live by a call which unwinds..

Ping.

Aug 10 2016, 7:48 AM

Aug 5 2016

eli.friedman added a comment to D22737: [SROA] Fix crash with lifetime intrinsic partially covering alloca..

Ping. (Not sure who else would be appropriate to review this.)

Aug 5 2016, 8:23 AM
eli.friedman added a comment to D21007: DSE: Don't remove stores made live by a call which unwinds..

Ping.

Aug 5 2016, 8:22 AM

Jul 31 2016

eli.friedman added inline comments to D19178: Broaden FoldItoFPtoI to try and establish whether the integer value fits into the float type.
Jul 31 2016, 3:50 PM
eli.friedman added inline comments to D19178: Broaden FoldItoFPtoI to try and establish whether the integer value fits into the float type.
Jul 31 2016, 3:03 PM
eli.friedman added inline comments to D23000: [X86] Improve 64-bit shifts on 32-bit targets (PR14593).
Jul 31 2016, 12:49 PM
eli.friedman accepted D23000: [X86] Improve 64-bit shifts on 32-bit targets (PR14593).

LGTM.

Jul 31 2016, 12:15 PM
eli.friedman added a comment to D23000: [X86] Improve 64-bit shifts on 32-bit targets (PR14593).

Might be nice to have a test which triggers this combine without going through legalization. (Should be possible, I think?)

Jul 31 2016, 8:51 AM

Jul 30 2016

eli.friedman added inline comments to D19178: Broaden FoldItoFPtoI to try and establish whether the integer value fits into the float type.
Jul 30 2016, 7:53 PM
eli.friedman added a comment to D19178: Broaden FoldItoFPtoI to try and establish whether the integer value fits into the float type.

Overall, this is looking much better; simple is good. :)

Jul 30 2016, 5:42 PM

Jul 29 2016

eli.friedman added a comment to D22933: DAG: avoid truncating a sign extended operand when test equality against zero .

Oh, I wasn't really paying attention to that... I guess it is? If it doesn't work, that's fine. :) The tests look better now.

Jul 29 2016, 5:05 PM
eli.friedman accepted D22933: DAG: avoid truncating a sign extended operand when test equality against zero .
Jul 29 2016, 3:15 PM
eli.friedman added inline comments to D22933: DAG: avoid truncating a sign extended operand when test equality against zero .
Jul 29 2016, 12:11 PM
eli.friedman added a comment to D22076: GVN: If X > Y is true, then replace X == Y with false and X != Y with true.

Yes, create a function for it. And in general, assume the answer to "should I create a function for this" is yes.

Jul 29 2016, 11:30 AM
eli.friedman added a comment to D22933: DAG: avoid truncating a sign extended operand when test equality against zero .

Oh, and yes, this version is what I was thinking of.

Jul 29 2016, 11:13 AM
eli.friedman added a reviewer for D22933: DAG: avoid truncating a sign extended operand when test equality against zero : eli.friedman.
Jul 29 2016, 11:12 AM
eli.friedman added a comment to D22933: DAG: avoid truncating a sign extended operand when test equality against zero .

It would be nice to have a couple testcases involving numbers which aren't zero.

Jul 29 2016, 11:11 AM

Jul 28 2016

eli.friedman added a comment to D22933: DAG: avoid truncating a sign extended operand when test equality against zero .

I think you missed the point of my suggestion... as long as you can prove both sides have enough sign bits, it doesn't matter what kind of node you're dealing with.

Jul 28 2016, 6:48 PM
eli.friedman added inline comments to D22933: DAG: avoid truncating a sign extended operand when test equality against zero .
Jul 28 2016, 5:16 PM
eli.friedman added inline comments to D22630: Loop rotation.
Jul 28 2016, 2:48 PM
eli.friedman added inline comments to D22630: Loop rotation.
Jul 28 2016, 2:08 PM
eli.friedman added inline comments to D22630: Loop rotation.
Jul 28 2016, 1:12 PM
eli.friedman added a comment to D22918: [Loop Vectorizer] Support predication of div/rem.

You might want to consider special-casing division by a constant integer. For example, on x86, we can convert a 16-bit unsigned divide by a constant into a pmulhuw+psrlw.

Jul 28 2016, 11:45 AM
eli.friedman added inline comments to D22630: Loop rotation.
Jul 28 2016, 8:57 AM

Jul 27 2016

eli.friedman added inline comments to D19178: Broaden FoldItoFPtoI to try and establish whether the integer value fits into the float type.
Jul 27 2016, 6:08 PM
eli.friedman added a comment to D22630: Loop rotation.

There may be another road if you think that would be better: we could keep the current loop rotation as is, and add a new loop rotation pass in a different file, and then deprecate the existing pass once everybody is convinced that the new code is better. Would that work for you?

Jul 27 2016, 1:07 PM
eli.friedman added inline comments to D22630: Loop rotation.
Jul 27 2016, 12:22 PM
eli.friedman added inline comments to D22630: Loop rotation.
Jul 27 2016, 11:58 AM
eli.friedman accepted D22850: AVX-512: Removed AssertZext node before TRUNCATE.

LGTM... with the caveat that I'm not sure I completely understand how the AVX-512 boolean registers work.

Jul 27 2016, 11:37 AM
eli.friedman added inline comments to D22630: Loop rotation.
Jul 27 2016, 11:21 AM

Jul 26 2016

eli.friedman added a comment to D22295: Introduce GlobalSplit pass..

I'll try to outline a scenario where this misbehaves...

Jul 26 2016, 9:10 PM
eli.friedman added a comment to D22295: Introduce GlobalSplit pass..

I don't see how unnamed_addr is relevant here. It means that the code won't compare the address of the global in question to the address of a different global. It doesn't have anything to do with comparisons involving different addresses within the same global.

Jul 26 2016, 8:40 PM
eli.friedman added inline comments to D22793: IR: Introduce inbounds attribute on getelementptr indices..
Jul 26 2016, 6:54 PM
eli.friedman added inline comments to D22793: IR: Introduce inbounds attribute on getelementptr indices..
Jul 26 2016, 6:01 PM
eli.friedman added inline comments to D22295: Introduce GlobalSplit pass..
Jul 26 2016, 3:51 PM
eli.friedman added inline comments to D22630: Loop rotation.
Jul 26 2016, 3:06 PM
eli.friedman added a comment to D22793: IR: Introduce inbounds attribute on getelementptr indices..

It's similar to TBAA, I guess... TBAA tends to cause problems too; fno-strict-aliasing is popular for a reason. :)

Jul 26 2016, 2:42 PM
eli.friedman added inline comments to D22295: Introduce GlobalSplit pass..
Jul 26 2016, 1:39 PM
eli.friedman added a comment to D22793: IR: Introduce inbounds attribute on getelementptr indices..

(I'll refer to the new attribute as inrange to try to avoid confusion.)

Jul 26 2016, 12:42 PM

Jul 25 2016

eli.friedman added inline comments to D19178: Broaden FoldItoFPtoI to try and establish whether the integer value fits into the float type.
Jul 25 2016, 12:17 PM
eli.friedman added inline comments to D19178: Broaden FoldItoFPtoI to try and establish whether the integer value fits into the float type.
Jul 25 2016, 10:52 AM
eli.friedman added inline comments to D22726: [DAGCombine] Match shift amount by value rather than relying on common sub-expressions..
Jul 25 2016, 10:48 AM

Jul 24 2016

eli.friedman added inline comments to D19178: Broaden FoldItoFPtoI to try and establish whether the integer value fits into the float type.
Jul 24 2016, 6:29 PM
eli.friedman added inline comments to D19178: Broaden FoldItoFPtoI to try and establish whether the integer value fits into the float type.
Jul 24 2016, 4:43 PM
eli.friedman added inline comments to D21460: [JumpThreading] Fix handling of aliasing metadata..
Jul 24 2016, 3:22 PM
eli.friedman added inline comments to D19178: Broaden FoldItoFPtoI to try and establish whether the integer value fits into the float type.
Jul 24 2016, 3:16 PM
eli.friedman added a comment to D21460: [JumpThreading] Fix handling of aliasing metadata..

Ping.

Jul 24 2016, 2:15 PM
eli.friedman updated the diff for D21007: DSE: Don't remove stores made live by a call which unwinds..

Don't use PointerMayBeCapturedBefore. Add some comments.

Jul 24 2016, 2:14 PM
eli.friedman retitled D22737: [SROA] Fix crash with lifetime intrinsic partially covering alloca. from to [SROA] Fix crash with lifetime intrinsic partially covering alloca..
Jul 24 2016, 2:10 PM
eli.friedman added a comment to D19178: Broaden FoldItoFPtoI to try and establish whether the integer value fits into the float type.

This is taking so many iterations to get to a reasonable place that I think you need some other approach to testing; you clearly aren't finding the interesting cases. Have you considered some sort of test generator, which randomly generates known one and known zero masks, then exhaustively checks whether the end result is consistent?

Jul 24 2016, 10:09 AM

Jul 23 2016

eli.friedman added inline comments to D22726: [DAGCombine] Match shift amount by value rather than relying on common sub-expressions..
Jul 23 2016, 3:32 PM

Jul 22 2016

eli.friedman added a comment to D21007: DSE: Don't remove stores made live by a call which unwinds..

InstrOrdering is basically an OrderedBasicBlock, I guess... I think I'd need to modify the interface a bit for this exact use to deal with modifications.

Jul 22 2016, 10:25 AM

Jul 21 2016

eli.friedman updated the diff for D21007: DSE: Don't remove stores made live by a call which unwinds..

Rebase on top of trunk.

Jul 21 2016, 7:59 PM
eli.friedman added a comment to D22652: GVH-hoist: only clone GEPs (PR28606) .

You're missing an allOperandsAvailable call if Val is a GEP. (I'm not sure why you're even trying to special-case GEPs here.)

Jul 21 2016, 4:29 PM
eli.friedman added inline comments to D22652: GVH-hoist: only clone GEPs (PR28606) .
Jul 21 2016, 3:28 PM
eli.friedman added a comment to D22377: [SCEV] trip count calculation for loops with unknown stride.

Re: the side-effect issue: You're trying to prove the loop will eventually either exit or execute undefined behavior. loopHasNoAbnormalExits proves that if the loop exits, it will exit via a visible edge (so it can't throw an exception or kill the program). There's a gap between the two: loopHasNoAbnormalExits doesn't care whether there's a nested infinite loop. In practice, this becomes an issue with atomic operations.

Jul 21 2016, 3:12 PM
eli.friedman added a comment to D19178: Broaden FoldItoFPtoI to try and establish whether the integer value fits into the float type.

Needs more comments in the code, and more tests. In particular, you have zero tests of values with trailing zeros.

Jul 21 2016, 2:40 PM
eli.friedman accepted D22537: [InstSimplify] recognize trunc + icmp sgt/slt variants of select simplifications (PR28466).

LGTM for now, I guess.

Jul 21 2016, 1:58 PM
eli.friedman added a comment to D22601: [SCCP] Mark constant xor %blah, %blah even if the lattice value is overdefined.

There, SCCP actually does add power to the transformation; consider a construct like the following:

Jul 21 2016, 1:17 PM
eli.friedman added a comment to D22601: [SCCP] Mark constant xor %blah, %blah even if the lattice value is overdefined.

The transformation valid... but I don't think it makes sense to perform it here. SCCP doesn't add any power to the transformation over instsimplify.

Jul 21 2016, 12:52 PM
eli.friedman added a comment to D22630: Loop rotation.

It looks like you're adding a bunch of bugpoint-reduced testcases without any comments... tests should at least explain what specifically they're trying to test.

Jul 21 2016, 11:13 AM

Jul 20 2016

eli.friedman accepted D22336: [SCCP] zap multiple return values.

Sorry about the delay. LGTM.

Jul 20 2016, 12:36 PM
eli.friedman accepted D22271: [InstCombine] reverse canonicalization of xor(zext i1 A), 1 <---> zext(not i1 A, true) (PR28476).

LGTM.

Jul 20 2016, 10:31 AM
eli.friedman added inline comments to D22537: [InstSimplify] recognize trunc + icmp sgt/slt variants of select simplifications (PR28466).
Jul 20 2016, 10:04 AM

Jul 19 2016

eli.friedman added a comment to D22537: [InstSimplify] recognize trunc + icmp sgt/slt variants of select simplifications (PR28466).

It looks like there are other places that look for bit-tests which are affected... see https://github.com/llvm-mirror/llvm/blob/dba9c3285e87768e80da6ca60f73b8913392dd98/lib/Transforms/InstCombine/InstCombineSelect.cpp#L579 , https://github.com/llvm-mirror/llvm/blob/65165b21da2aaed908f7bde1b8a16e7a8fe0c955/lib/Transforms/InstCombine/InstCombineAndOrXor.cpp#L597 . Maybe we want a separate utility function to condense this functionality, so we don't end up with so many versions of it?

Jul 19 2016, 4:26 PM
eli.friedman accepted D22105: [X86][SSE] Reimplement SSE fp2si conversion intrinsics instead of using generic IR.

LGTM.

Jul 19 2016, 12:13 PM
eli.friedman added a comment to D22105: [X86][SSE] Reimplement SSE fp2si conversion intrinsics instead of using generic IR.

The x86-specific operation is affected by the rounding mode... but so is a C cast. This is specified by Annex F in the C standard.

Jul 19 2016, 10:42 AM
eli.friedman added a comment to D22105: [X86][SSE] Reimplement SSE fp2si conversion intrinsics instead of using generic IR.

I don't think we need to use x86-specific operations for sitofp-like conversions; the C cast is equivalent given that a 32 or 64-bit integer is always in within the range of a 32-bit float.

Jul 19 2016, 10:16 AM

Jul 18 2016

eli.friedman accepted D22106: [X86][SSE] Reimplement SSE fp2si conversion intrinsics instead of using generic IR.

LGTM.

Jul 18 2016, 10:25 AM

Jul 14 2016

eli.friedman added a comment to D22377: [SCEV] trip count calculation for loops with unknown stride.

Oh, right, missed the ControlsExit. That doesn't help with the exception-throwing case, though.

Jul 14 2016, 1:43 PM
eli.friedman added a comment to D22377: [SCEV] trip count calculation for loops with unknown stride.

You have to be a bit more careful here... consider:

Jul 14 2016, 1:00 PM

Jul 13 2016

eli.friedman added inline comments to D22336: [SCCP] zap multiple return values.
Jul 13 2016, 10:54 PM
eli.friedman added a comment to D22295: Introduce GlobalSplit pass..

The semantics you've implemented seem extremely dangerous. You're making some very strong assumptions about how exactly the optimizer manipulates GEPs. I'm not sure whether the optimizer actually breaks this particular construct, but it easily could. The optimizer can and will dig through a ConstantExpr and transform the use into something different.

Jul 13 2016, 9:52 PM
eli.friedman accepted D22329: [IPSCCP] Constant fold struct argument/instructions when all the lattice values are constant.

LGTM.

Jul 13 2016, 7:53 PM
eli.friedman added inline comments to D20116: Add speculatable function attribute.
Jul 13 2016, 3:48 PM

Jul 12 2016

eli.friedman added a comment to D22114: [InstCombine] extend vector select matching for non-splat constants.

The question isn't really what instructions we expect targets to have, but rather which version makes LLVM simpler overall. That said, if we assume almost every vector target has some sort of blend instruction, and therefore almost every target would need to pattern match the shuffle into a select, that would be a reason to prefer the select form, I guess.

Jul 12 2016, 5:32 PM
eli.friedman added a comment to D22114: [InstCombine] extend vector select matching for non-splat constants.

I would lean towards making shufflevector canonical. It interacts better with existing transforms involving shuffles and vector insert/extract. I can't think of any benefit to making the select form canonical.

Jul 12 2016, 3:41 PM
eli.friedman accepted D22114: [InstCombine] extend vector select matching for non-splat constants.

LGTM.

Jul 12 2016, 1:21 PM
eli.friedman added a comment to D22271: [InstCombine] reverse canonicalization of xor(zext i1 A), 1 <---> zext(not i1 A, true) (PR28476).

Is "zext(a_bool ^ true) ^ 2" canonical, or is "zext(a_bool) ^ 3"? Either way, you're missing a transform.

Jul 12 2016, 1:15 PM
eli.friedman accepted D22269: [SCCP] Replace structs with constants if all the lattice values are constant.

LGTM.

Jul 12 2016, 12:55 PM
eli.friedman added a comment to D22269: [SCCP] Replace structs with constants if all the lattice values are constant.

You probably want to refactor this if you're planning to use it from IPSCCP... but you can do that in the followup, I guess.

Jul 12 2016, 11:39 AM

Jul 9 2016

eli.friedman accepted D22192: [SCCP] Change isUndefined() -> isUnknown().

LGTM.

Jul 9 2016, 5:07 PM
eli.friedman added a comment to D22186: [SCCP] Add support for insertelement instructions.

Sure, that's fine (in a separate commit, of course).

Jul 9 2016, 3:56 PM
eli.friedman added a comment to D22186: [SCCP] Add support for insertelement instructions.

I think you're misunderstanding what "isUndefined" means... it means "we haven't computed the value of this instruction yet", not "this value is an UndefValue". See https://www.cs.rice.edu/~keith/512/2011/Lectures/L19-SCCP-1up.pdf and http://citeseerx.ist.psu.edu/viewdoc/download?doi=10.1.1.105.4146&rep=rep1&type=pdf .

Jul 9 2016, 3:19 PM

Jul 8 2016

eli.friedman added a comment to D22122: [SCCP] Teach the pass about bitcasts.

SCCP doesn't handle vectors. There's some code for it but it's #if 0'd . I re-enabled that code and tried to build and it passes test suite and it's able to self-host LLVM, so, I'm not entirely sure why that code is disabled and the comments in the code don't help :| Do you happen to know what's the reason?

Jul 8 2016, 12:36 PM

Jul 7 2016

eli.friedman added a comment to D22122: [SCCP] Teach the pass about bitcasts.

I guess it's a good idea to try to fold constants as we build them... it's slightly more efficient, and it could avoid spurious "overdefined" markings in some cases. Might as well make a pass over the whole file while you're at it to find other places where we do something similar.

Jul 7 2016, 6:50 PM
eli.friedman added a comment to D22122: [SCCP] Teach the pass about bitcasts.

SCCPSolver::visitCastInst already handles bitcasts... what are you trying to do?

Jul 7 2016, 5:42 PM
eli.friedman added inline comments to D22114: [InstCombine] extend vector select matching for non-splat constants.
Jul 7 2016, 4:41 PM
eli.friedman added inline comments to D22106: [X86][SSE] Reimplement SSE fp2si conversion intrinsics instead of using generic IR.
Jul 7 2016, 4:40 PM
eli.friedman added a comment to D22114: [InstCombine] extend vector select matching for non-splat constants.

Do we care at all about the number of uses of the operands?

Jul 7 2016, 3:29 PM
eli.friedman added inline comments to D22106: [X86][SSE] Reimplement SSE fp2si conversion intrinsics instead of using generic IR.
Jul 7 2016, 3:24 PM

Jul 6 2016

eli.friedman added inline comments to D22008: GlobalISel: implement low-level type suitable for MachineInstr selection.
Jul 6 2016, 6:59 PM
eli.friedman added a comment to D22076: GVN: If X > Y is true, then replace X == Y with false and X != Y with true.

Please don't copy-paste code.

Jul 6 2016, 5:41 PM
eli.friedman added a comment to D22074: GlobalISel: legalization policy class.

Are you sure you uploaded the right patch here? This isn't against trunk, as far as I can tell.

Jul 6 2016, 5:35 PM
eli.friedman accepted D20638: [LIR] Fix mis-compilation with unwinding.

LGTM.

Jul 6 2016, 11:37 AM
eli.friedman accepted D21613: [DSE] Avoid iterator invalidation bugs..

LGTM.

Jul 6 2016, 11:36 AM
eli.friedman accepted D21899: [InstCombine] extend (select X, C1, C2 --> ext X) to vectors.

LGTM.

Jul 6 2016, 11:16 AM
eli.friedman added a comment to D20638: [LIR] Fix mis-compilation with unwinding.

The code looks fine. The comments could use a bit of refinement.

Jul 6 2016, 9:53 AM
eli.friedman added a comment to D21928: Aliasing of constant pointers (inttoptr Const) for BasicAA.

Please ignore my previous comments, they were overly terse and not quite correct.

Jul 6 2016, 9:37 AM

Jul 5 2016

eli.friedman added a comment to D21613: [DSE] Avoid iterator invalidation bugs..

Sorry about the delay; I thought I had sent review comments, but apparently they were saved/unsent.

Jul 5 2016, 10:46 AM