Page MenuHomePhabricator

modocache (Brian Gesiak)
User

Projects

User does not belong to any projects.

User Details

User Since
Feb 16 2014, 1:10 AM (361 w, 4 d)

Recent Activity

Dec 17 2020

modocache committed rG14f24155a591: [mlir][LLVMIR] Add 'llvm.switch' op (authored by modocache).
[mlir][LLVMIR] Add 'llvm.switch' op
Dec 17 2020, 11:17 AM
modocache closed D93005: [mlir][LLVMIR] Add 'llvm.switch' op.
Dec 17 2020, 11:17 AM · Restricted Project

Dec 16 2020

modocache added a comment to D93005: [mlir][LLVMIR] Add 'llvm.switch' op.

I think I'll commit this later today, but let me know if you have any thoughts on adding an interface for indenting text to the printer, @rriddle. I'd be happy to try my hand at sending a patch to do so, as well.

Dec 16 2020, 7:29 AM · Restricted Project

Dec 15 2020

modocache added a comment to D93005: [mlir][LLVMIR] Add 'llvm.switch' op.

Not at all, thank you for all the great feedback!

Dec 15 2020, 7:47 AM · Restricted Project
modocache updated the diff for D93005: [mlir][LLVMIR] Add 'llvm.switch' op.

Two small tweaks: give the BlockRange caseDestinations parameter of the op builder a default empty value, and "indent" cases before printing them.

Dec 15 2020, 7:39 AM · Restricted Project

Dec 14 2020

modocache updated the diff for D93005: [mlir][LLVMIR] Add 'llvm.switch' op.

Fix comment typo in test.

Dec 14 2020, 2:22 PM · Restricted Project
modocache updated the diff for D93005: [mlir][LLVMIR] Add 'llvm.switch' op.

Thanks for the integer and newline patches @rriddle! I've updated this patch to use those new features. And thanks for another round of reviews @ftynse, I think I've responded to all the feedback given so far, let me know if I've missed anything.

Dec 14 2020, 2:02 PM · Restricted Project
modocache planned changes to D93005: [mlir][LLVMIR] Add 'llvm.switch' op.

Sounds good, thanks! I agree, I'll modify this patch later today such that the MLIR llvm.switch op accepts zero cases, just as LLVM IR switch instruction does.

Dec 14 2020, 11:39 AM · Restricted Project

Dec 12 2020

modocache committed rG09b0e0884a3e: [mlir] Print bad size in AttrSizedOperandSegments (authored by modocache).
[mlir] Print bad size in AttrSizedOperandSegments
Dec 12 2020, 10:13 AM
modocache closed D93145: [mlir] Print bad size in AttrSizedOperandSegments.
Dec 12 2020, 10:13 AM · Restricted Project

Dec 11 2020

modocache updated the diff for D93005: [mlir][LLVMIR] Add 'llvm.switch' op.

Rebase past D92845, add instantiated SwitchInst to the branch mapping.

Dec 11 2020, 4:05 PM · Restricted Project
modocache added inline comments to D93005: [mlir][LLVMIR] Add 'llvm.switch' op.
Dec 11 2020, 3:24 PM · Restricted Project
modocache updated the diff for D93005: [mlir][LLVMIR] Add 'llvm.switch' op.

Thanks for answering all my questions! I think I've responded to all of your feedback, let me know if I missed anything. I also have a question about parsing and printing integers that I'll post as an inline comment.

Dec 11 2020, 3:19 PM · Restricted Project
modocache requested review of D93145: [mlir] Print bad size in AttrSizedOperandSegments.
Dec 11 2020, 3:00 PM · Restricted Project
modocache planned changes to D93005: [mlir][LLVMIR] Add 'llvm.switch' op.
Dec 11 2020, 1:39 PM · Restricted Project
modocache updated the diff for D93005: [mlir][LLVMIR] Add 'llvm.switch' op.

So, unfortunately I don't think it's feasible to use the declarative assembly format for this op, at least not without making some TableGen modifications to pass custom directives a reference to the OperationState. I'm also not certain it's possible to get rid of case_operand_offsets in favor of operand_segment_sizes, since this op seems unique in storing a variable list of variadic operands. But, aside from those two points, I think I've addressed all of your review comments so far. Thank you, @ftynse and @rriddle! Please take another look at this when you have some time.

Dec 11 2020, 1:16 PM · Restricted Project
modocache added inline comments to D93005: [mlir][LLVMIR] Add 'llvm.switch' op.
Dec 11 2020, 12:55 PM · Restricted Project
modocache updated the diff for D93005: [mlir][LLVMIR] Add 'llvm.switch' op.

Add to the test for operand segment sizes.

Dec 11 2020, 11:19 AM · Restricted Project
modocache updated the diff for D93005: [mlir][LLVMIR] Add 'llvm.switch' op.

Oops, forgot to clang-format.

Dec 11 2020, 11:05 AM · Restricted Project
modocache updated the diff for D93005: [mlir][LLVMIR] Add 'llvm.switch' op.

I'm adding an unrelated change to the AttrSizedOperandSegments verifier that prints how many elements it found in operand_segment_sizes. It's not necessarily part of my patch, but it helped me when debugging. I'd be happy to split it out into its own if anyone would prefer that.

Dec 11 2020, 10:24 AM · Restricted Project
modocache requested review of D93005: [mlir][LLVMIR] Add 'llvm.switch' op.
Dec 11 2020, 10:20 AM · Restricted Project
modocache planned changes to D93005: [mlir][LLVMIR] Add 'llvm.switch' op.
Dec 11 2020, 8:12 AM · Restricted Project

Dec 10 2020

modocache added inline comments to D93005: [mlir][LLVMIR] Add 'llvm.switch' op.
Dec 10 2020, 1:13 PM · Restricted Project
modocache added inline comments to D93005: [mlir][LLVMIR] Add 'llvm.switch' op.
Dec 10 2020, 1:09 PM · Restricted Project
modocache updated the diff for D93005: [mlir][LLVMIR] Add 'llvm.switch' op.

Thanks for the review, @ftynse!

Dec 10 2020, 1:09 PM · Restricted Project

Dec 9 2020

modocache requested review of D93005: [mlir][LLVMIR] Add 'llvm.switch' op.
Dec 9 2020, 11:52 PM · Restricted Project

Jul 15 2020

modocache accepted D83788: Removed unused variable in clang.

No problem, thanks for this!

Jul 15 2020, 8:23 AM · Restricted Project

Jul 14 2020

modocache added a comment to D83788: Removed unused variable in clang.

By the way, I tried to accept this diff and leave the following inline comment on SemaExpr.cpp:15799:

Jul 14 2020, 11:25 AM · Restricted Project
modocache added a comment to D83788: Removed unused variable in clang.

This LGTM! Looks like the last time this variable was touched was in 2010 as part of a mechanical renaming, https://github.com/llvm/llvm-project/commit/a771f46c82d7. At that point it was used, as a parameter to the static function MakeObjCStringLiteralFixItHint. That static function was later renamed on Dec 17 2013 https://github.com/llvm/llvm-project/commit/bd714e9bb12, to ConversionToObjCStringLiteralCheck, and on the following day Dec 18 2013 in https://github.com/llvm/llvm-project/commit/283bf895066 the signature was changed to no longer take a FixitHint parameter. The author must not have removed this variable at that point because technically it is still referenced, but as you point out, it's never mutated in a way that gives the hint a value.

Jul 14 2020, 11:24 AM · Restricted Project

Jul 1 2020

modocache accepted D82986: [Coroutines] Fix test breakage in D82928.

Thanks for the fix!

Jul 1 2020, 11:20 AM · Restricted Project
modocache accepted D82928: [Coroutines] Fix code coverage for coroutine.

Excellent, thanks for this!

Jul 1 2020, 8:37 AM · Restricted Project

Jun 25 2020

modocache accepted D82332: [Coroutines] Handle dependent promise types for final_suspend non-throw check.

Thanks, this looks good to me!

Jun 25 2020, 11:20 AM · Restricted Project

Jun 22 2020

modocache accepted D82029: [Coroutines] Ensure co_await promise.final_suspend() does not throw.

Sweet! Thanks for the reviews/responses, LGTM :)

Jun 22 2020, 12:21 PM · Restricted Project

Jun 18 2020

modocache requested changes to D81885: [Coroutines] Return false on error of buildSuspends.

I don't have a preference as to whether Sema::ActOnCoroutineBodyStart returns true or false to indicate failure, although I think returning true for failures is a bit of a pre-existing convention, such as here, where true is returned for an invalid declaration: https://github.com/llvm/llvm-project/blob/84167a8d58e8af79625abcffdf2c860d556955e6/clang/lib/Sema/SemaDecl.cpp#L1572

Jun 18 2020, 9:45 AM · Restricted Project
modocache added a reviewer for D82029: [Coroutines] Ensure co_await promise.final_suspend() does not throw: junparser.

Excellent, thank you! The test failures on the diff appear to be legitimate, they reproduce for me when I apply this patch to my local checkout and run ninja check-clang. Could you take a look?

Jun 18 2020, 5:57 AM · Restricted Project

Apr 4 2020

modocache committed rG54176d1766f2: libcxx 'LLVM_USE_SANITIZER=Address;Undefined' (authored by modocache).
libcxx 'LLVM_USE_SANITIZER=Address;Undefined'
Apr 4 2020, 1:50 PM
modocache closed D77466: libcxx 'LLVM_USE_SANITIZER=Address;Undefined'.
Apr 4 2020, 1:50 PM · Restricted Project
modocache created D77466: libcxx 'LLVM_USE_SANITIZER=Address;Undefined'.
Apr 4 2020, 10:37 AM · Restricted Project

Apr 2 2020

modocache committed rG627e01feb718: [coroutines] Don't build promise init with no args (authored by modocache).
[coroutines] Don't build promise init with no args
Apr 2 2020, 7:00 PM
modocache closed D70555: [coroutines] Don't build promise init with no args.
Apr 2 2020, 7:00 PM · Restricted Project
modocache added a comment to D70555: [coroutines] Don't build promise init with no args.

Of course, your approval is very welcome! 😄 I'll go ahead and land this today, thanks for the review!

Apr 2 2020, 10:50 AM · Restricted Project
modocache accepted D77035: [Coroutines] Simplify implementation using removePredecessor.

LGTM, thanks!

Apr 2 2020, 10:50 AM · Restricted Project
modocache abandoned D76016: [ProfileData] Add option to ignore invalid regions.

Yup, makes sense. In the codebase I maintain, I think there are a few distinct coverage issues, so I'll keep this patch applied to my fork of LLVM for the time being, until I send patches to fix those issues. But it sounds like this isn't something that we'd want in LLVM trunk, so I'll close this out. Thanks!

Apr 2 2020, 10:50 AM · Restricted Project

Mar 28 2020

modocache accepted D76913: [Coroutines 2/2] Improve symmetric control transfer feature .

Looks great, thanks!

Mar 28 2020, 5:44 PM · Restricted Project
modocache accepted D76911: [Coroutines 1/2] Improve symmetric control transfer feature .

Looks good to me! Initially I was wondering about whether shouldBeMustTail ought to check whether the next instruction is a ret, or a bitcast that's used by a ret, but I see simplifyTerminatorLeadingToRet takes care of that check. Thanks for the patch!

Mar 28 2020, 5:44 PM · Restricted Project

Mar 19 2020

modocache accepted D76345: [Coroutines] Fix PR45130.

Excellent, thank you! I had one question, otherwise this looks great!

Mar 19 2020, 7:45 PM · Restricted Project
modocache added a comment to D76016: [ProfileData] Add option to ignore invalid regions.

I see. No, I think whenever we have malformed coverage data, it points to an underlying bug in the compiler. Still, I think llvm-cov's treatment of invalid data is too severe. Is there a good reason it should bail entirely? If it's able to process all but a single coverage region, it seems like it ought to report what it was able to process, no? I think that at least having the option for llvm-cov to behave this way is helpful, although I can understand if others disagree.

Mar 19 2020, 1:42 PM · Restricted Project

Mar 18 2020

modocache added a comment to D75579: Replace MCTargetOptionsCommandFlags.inc and CommandFlags.inc by runtime-registration.

Awesome, thank you!

Mar 18 2020, 8:42 AM · Restricted Project, Restricted Project

Mar 17 2020

modocache added a comment to D75579: Replace MCTargetOptionsCommandFlags.inc and CommandFlags.inc by runtime-registration.

I may be wrong, but I believe this breaks check-llvm when run with the following configuration: cmake -DLLVM_BINUTILS_INCDIR=/path/to/binutils/include:

Mar 17 2020, 7:59 PM · Restricted Project, Restricted Project

Mar 13 2020

modocache added reviewers for D70555: [coroutines] Don't build promise init with no args: junparser, wenlei.
Mar 13 2020, 7:28 AM · Restricted Project
modocache abandoned D75542: [Sema] Prevent UB for uninitialized `IsSurrogate`.

Alternatively, I considered modifying the clang::OverloadCandidate constructor to initialize IsSurrogate with a false value. I feel doing so would be safer, but I stuck with what appears to be the convention: initializing OverloadCandidate members are the site of use.

Mar 13 2020, 7:28 AM · Restricted Project
modocache added inline comments to D76119: [Coroutines] Insert lifetime intrinsics even O0 is used.
Mar 13 2020, 6:22 AM · Restricted Project
modocache accepted D75664: [Coroutines] Also check lifetime intrinsic for local variable when build coroutine frame.

Nice, thanks for this improvement! Am I right in thinking patch is dependent upon D76119? My understanding is that, without that patch, then we wouldn't be able to rely on lifetime intrinsics when using -O0. You may want to use Phabricator's "Edit Related Objects" link to specify that this patch depends on that one.

Mar 13 2020, 6:22 AM · Restricted Project
modocache requested changes to D76119: [Coroutines] Insert lifetime intrinsics even O0 is used.

Nice, thank you. I like the test case you added, it feels more reduced than the one in D76118 -- even if it does use long type names such as awaitable, they make the test case easy to understand. But I had a suggestion to expand upon the test case that I think would help this patch.

Mar 13 2020, 5:46 AM · Restricted Project
modocache accepted D76118: [Coroutines] Do not evaluate InitListExpr of a co_return.

Awesome, thank you! This is an issue I've been running into as well in an internal codebase, I'll check whether this patch addresses it.

Mar 13 2020, 5:46 AM · Restricted Project

Mar 12 2020

modocache added a comment to D75542: [Sema] Prevent UB for uninitialized `IsSurrogate`.

Friendly ping! OverloadCandidate has uninitialized members and so can cause UB if used incorrectly in another part of the compiler, so I feel this is a fairly straightforward patch: prevent UB by initializing all members. But I'd like some review to confirm. @rsmith? @aaron.ballman?

Mar 12 2020, 11:23 AM · Restricted Project

Mar 11 2020

modocache updated the summary of D76016: [ProfileData] Add option to ignore invalid regions.
Mar 11 2020, 11:52 AM · Restricted Project
modocache created D76016: [ProfileData] Add option to ignore invalid regions.
Mar 11 2020, 11:22 AM · Restricted Project

Mar 4 2020

modocache added a comment to D74516: Extend TimeTrace to LLVM's new pass manager.

Friendly ping! New PM guys, maybe @chandlerc or @lebedev.ri could review?

Mar 4 2020, 2:05 PM · Restricted Project
modocache accepted D75440: [Coroutines] Optimized coroutine elision based on reachability.
Mar 4 2020, 9:00 AM · Restricted Project

Mar 3 2020

modocache added a comment to D75440: [Coroutines] Optimized coroutine elision based on reachability.

yep, I have tried the two samples, same behavior as clang6.

Mar 3 2020, 3:00 PM · Restricted Project
modocache committed rGaa85b437a970: [Coroutines] Use dbg.declare for frame variables (authored by modocache).
[Coroutines] Use dbg.declare for frame variables
Mar 3 2020, 2:26 PM
modocache closed D75338: [Coroutines] Use dbg.declare for frame variables.
Mar 3 2020, 2:25 PM · Restricted Project
modocache updated the diff for D75338: [Coroutines] Use dbg.declare for frame variables.

Thanks again for the review. I moved findDbgDeclareUses to Local.h (no unittest for now but I'll add one in a follow-up diff for both it and FindDbgAddrUses, which doesn't have one either) and I removed the redundant loop. I'll commit this now and add an lldb regression test in a follow-up patch.

Mar 3 2020, 2:24 PM · Restricted Project
modocache added inline comments to D75338: [Coroutines] Use dbg.declare for frame variables.
Mar 3 2020, 2:24 PM · Restricted Project
modocache added inline comments to D75338: [Coroutines] Use dbg.declare for frame variables.
Mar 3 2020, 12:37 PM · Restricted Project
modocache added inline comments to D75542: [Sema] Prevent UB for uninitialized `IsSurrogate`.
Mar 3 2020, 10:21 AM · Restricted Project
modocache created D75542: [Sema] Prevent UB for uninitialized `IsSurrogate`.
Mar 3 2020, 10:16 AM · Restricted Project

Mar 2 2020

modocache added inline comments to D75338: [Coroutines] Use dbg.declare for frame variables.
Mar 2 2020, 7:30 PM · Restricted Project
modocache updated the diff for D75338: [Coroutines] Use dbg.declare for frame variables.

Thanks for the review, @jmorse! I responded to your comments: I delete only dbg.declare, replace other uses with undef, and I removed unnecessary attributes.

Mar 2 2020, 7:30 PM · Restricted Project
modocache added a comment to D75338: [Coroutines] Use dbg.declare for frame variables.

They are not broken [1], but you may need to add libcxx and friends to your list of projects in your LLVM cmake invocation.

Mar 2 2020, 7:03 PM · Restricted Project
modocache added a comment to D75440: [Coroutines] Optimized coroutine elision based on reachability.

Thank you for this patch, I'll give it a proper review tonight. But in the meantime, I wonder if you've tried this with any of the sample programs for this bug report, https://bugs.llvm.org/show_bug.cgi?id=40656 ?

Mar 2 2020, 2:56 PM · Restricted Project

Mar 1 2020

modocache updated the diff for D75338: [Coroutines] Use dbg.declare for frame variables.

Add checks for !dbg locations.

Mar 1 2020, 9:17 PM · Restricted Project
modocache added a comment to D75338: [Coroutines] Use dbg.declare for frame variables.

I tried adding an lldb test for this, but I encountered 39 failing tests when I ran ninja cxx check-lldb, and it made me wonder whether tests that use libc++ are currently broken?

Mar 1 2020, 8:56 PM · Restricted Project

Feb 28 2020

modocache accepted D75345: [Coroutines][new pass manager] Move CoroElide pass to right position.

Nice catch, thanks!

Feb 28 2020, 1:15 PM · Restricted Project
modocache added a comment to D75338: [Coroutines] Use dbg.declare for frame variables.

As a side note: Are you using LLDB as your debugger? We could probably use a couple of end-to-end tests in the LLDB testsuite for coroutines as well if you are interested in contributing one.

Feb 28 2020, 10:05 AM · Restricted Project
modocache updated the diff for D75338: [Coroutines] Use dbg.declare for frame variables.

Remove Git commit hashes from the debug info metadata, just to slim it down a tiny bit more.

Feb 28 2020, 7:31 AM · Restricted Project
modocache updated the diff for D75338: [Coroutines] Use dbg.declare for frame variables.

I played around a bit more and got it down to just 13 nodes. I think this is the bare minimum.

Feb 28 2020, 7:30 AM · Restricted Project
modocache updated the diff for D75338: [Coroutines] Use dbg.declare for frame variables.

Wow, great suggestion, @dstenb! That brought the number of nodes down to 119 -- a reduction of 82%! (Also, I ran clang-format on the include files to appease the pre-merge checkers -- although to be honest, I'm not sure I agree with the rationale here, because this diff didn't modify the include that clang-format is complaining about. But oh well.)

Feb 28 2020, 7:17 AM · Restricted Project
modocache added inline comments to D75338: [Coroutines] Use dbg.declare for frame variables.
Feb 28 2020, 4:13 AM · Restricted Project
modocache created D75338: [Coroutines] Use dbg.declare for frame variables.
Feb 28 2020, 4:13 AM · Restricted Project

Feb 23 2020

modocache accepted D71663: [Coroutines] CoroElide enhancement .

Sorry this review took so long, and thank you for addressing the regression I introduced in D43242!

Feb 23 2020, 7:45 AM · Restricted Project

Feb 19 2020

modocache added a comment to D74516: Extend TimeTrace to LLVM's new pass manager.

Friendly ping for reviews on this one. I've found this to be a great tool for debugging the passes running during ThinLTO.

Feb 19 2020, 6:05 PM · Restricted Project
modocache accepted D74514: Refactor TimeProfiler write methods (NFC).

This looks good to me, it sounds like all of @rnk's comments were addressed, and I'd like to unblock D74516 that's stacked on top of this, so I'm going to accept. @amonshiz do you want me to commit this for you? (And @rnk, any comments before I do?)

Feb 19 2020, 5:56 PM · Restricted Project

Feb 18 2020

modocache committed rG048239e46e49: [Coroutines][6/6] Clang schedules new passes (authored by modocache).
[Coroutines][6/6] Clang schedules new passes
Feb 18 2020, 10:10 PM
modocache closed D71903: [Coroutines][6/6] Clang schedules new passes.
Feb 18 2020, 10:10 PM · Restricted Project
modocache committed rG72961071f35a: [Coroutines][5/6] Add coroutine passes to pipeline (authored by modocache).
[Coroutines][5/6] Add coroutine passes to pipeline
Feb 18 2020, 10:05 PM
modocache closed D71902: [Coroutines][5/6] Add coroutine passes to pipeline.
Feb 18 2020, 10:04 PM · Restricted Project
modocache committed rG5a187d8ed11f: [Coroutines][4/6] New pass manager: coro-cleanup (authored by modocache).
[Coroutines][4/6] New pass manager: coro-cleanup
Feb 18 2020, 9:42 PM
modocache closed D71901: [Coroutines][4/6] New pass manager: coro-cleanup.
Feb 18 2020, 9:42 PM · Restricted Project
modocache committed rG2365238b9d0b: Re-land new pass manager coro-split and coro-elide (authored by modocache).
Re-land new pass manager coro-split and coro-elide
Feb 18 2020, 9:20 PM
modocache committed rG26f356350bd5: [LazyCallGraph] Fix ambiguous index value (authored by modocache).
[LazyCallGraph] Fix ambiguous index value
Feb 18 2020, 8:36 PM
modocache committed rGc30d8f7c910d: [IR] Set name when inserting 'llvm::Value*' (authored by modocache).
[IR] Set name when inserting 'llvm::Value*'
Feb 18 2020, 5:23 AM
modocache closed D74754: [IR] Set name when inserting 'llvm::Value*'.
Feb 18 2020, 5:23 AM · Restricted Project
modocache updated the diff for D74754: [IR] Set name when inserting 'llvm::Value*'.

Thanks for the reviews! Good point about the variable casing, I changed it and am going to commit this shortly.

Feb 18 2020, 5:22 AM · Restricted Project

Feb 17 2020

modocache committed rGe999aa38d169: Revert new files from new pass manager coro-split/coro-elide (authored by modocache).
Revert new files from new pass manager coro-split/coro-elide
Feb 17 2020, 9:38 PM
modocache added a reverting change for rG11053a1cc61a: Revert new pass manager coro-split and coro-elide: rGe999aa38d169: Revert new files from new pass manager coro-split/coro-elide.
Feb 17 2020, 9:37 PM
modocache committed rG11053a1cc61a: Revert new pass manager coro-split and coro-elide (authored by modocache).
Revert new pass manager coro-split and coro-elide
Feb 17 2020, 8:57 PM
modocache committed rG00fec8004aca: [Coroutines][3/6] New pass manager: coro-elide (authored by modocache).
[Coroutines][3/6] New pass manager: coro-elide
Feb 17 2020, 8:48 PM
modocache closed D71900: [Coroutines][3/6] New pass manager: coro-elide.
Feb 17 2020, 8:48 PM · Restricted Project