Page MenuHomePhabricator

modocache (Brian Gesiak)
User

Projects

User does not belong to any projects.

User Details

User Since
Feb 16 2014, 1:10 AM (349 w, 6 d)

Recent Activity

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
modocache updated the diff for D71900: [Coroutines][3/6] New pass manager: coro-elide.

Thanks for the review! Yeah, you're right. I think the comment at the top of the file explains both the legacy and new pass manager just fine, so I removed the extra comment I'd added.

Feb 17 2020, 8:42 PM · Restricted Project
modocache committed rG7125d66f9969: [Coroutines][2/6] New pass manager: coro-split (authored by modocache).
[Coroutines][2/6] New pass manager: coro-split
Feb 17 2020, 8:39 PM
modocache closed D71899: [Coroutines][2/6] New pass manager: coro-split.
Feb 17 2020, 8:38 PM · Restricted Project
modocache updated the diff for D71899: [Coroutines][2/6] New pass manager: coro-split.

Thanks for the reviews! You're right, @jdoerfert, the file header comment is still totally applicable. I put it back in.

Feb 17 2020, 8:29 PM · Restricted Project
modocache added inline comments to D73835: [IRBuilder] Virtualize IRBuilder.
Feb 17 2020, 8:17 PM · Restricted Project, Restricted Project
modocache created D74754: [IR] Set name when inserting 'llvm::Value*'.
Feb 17 2020, 8:14 PM · Restricted Project
modocache added a reviewer for D71903: [Coroutines][6/6] Clang schedules new passes: wenlei.
Feb 17 2020, 4:07 PM · Restricted Project
modocache updated the diff for D71903: [Coroutines][6/6] Clang schedules new passes.

Rebase on top of the latest version of D71902.

Feb 17 2020, 4:07 PM · Restricted Project
modocache updated the diff for D71902: [Coroutines][5/6] Add coroutine passes to pipeline.

Updated to use a new approach: instead of a custom pipeline parser, '-enable-coroutines' now adds coroutine passes to the new pass manager's pass builder. Using this in conjunction with the 'default<O2>' pipeline parser, this patch can now run all coroutine tests in the LLVM test suite -- and they all pass.

Feb 17 2020, 3:49 PM · Restricted Project
modocache committed rG0deef2e164e1: Re-land "Add LazyCallGraph API to add function to RefSCC" (authored by modocache).
Re-land "Add LazyCallGraph API to add function to RefSCC"
Feb 17 2020, 2:00 PM
modocache committed rG28213680b2a7: Revert "Add LazyCallGraph API to add function to RefSCC" (authored by modocache).
Revert "Add LazyCallGraph API to add function to RefSCC"
Feb 17 2020, 11:26 AM
modocache committed rGe9849d5195e9: [Coroutines][1/6] New pass manager: coro-early (authored by modocache).
[Coroutines][1/6] New pass manager: coro-early
Feb 17 2020, 10:31 AM
modocache closed D71898: [Coroutines][1/6] New pass manager: coro-early.
Feb 17 2020, 10:31 AM · Restricted Project
modocache committed rG449a13509190: Add LazyCallGraph API to add function to RefSCC (authored by modocache).
Add LazyCallGraph API to add function to RefSCC
Feb 17 2020, 10:03 AM
modocache closed D72226: Add LazyCallGraph API to add function to RefSCC.
Feb 17 2020, 10:03 AM · Restricted Project
modocache updated the diff for D72226: Add LazyCallGraph API to add function to RefSCC.

Thanks for the reviews, @jdoerfert! I addressed your comment by removing the redundant 'completely'. I'll commit this in a few minutes.

Feb 17 2020, 9:54 AM · Restricted Project

Feb 15 2020

modocache accepted D74674: Coroutines: avoid use of deprecated CreateLoad and CreateCall methods.

This seems like a straightforward improvement to me (assuming all the tests still pass of course). Thanks for cleaning this up!

Feb 15 2020, 1:04 PM · Restricted Project

Feb 14 2020

modocache updated the diff for D71899: [Coroutines][2/6] New pass manager: coro-split.

I found a test that tested the legacy PM opt -coro-split, but not the new PM opt -passes=coro-split. Now it tests both.

Feb 14 2020, 10:03 AM · Restricted Project

Feb 13 2020

modocache updated the diff for D71903: [Coroutines][6/6] Clang schedules new passes.

Clean up coroutine intrinsics as part of the ThinLTO pre-link pipeline.

Feb 13 2020, 8:24 PM · Restricted Project
modocache updated the diff for D71899: [Coroutines][2/6] New pass manager: coro-split.

Rebase past D69930 so the patch applies cleanly to trunk.

Feb 13 2020, 8:16 PM · Restricted Project
modocache updated the diff for D72226: Add LazyCallGraph API to add function to RefSCC.

Thanks, @jdoerfert! As per your suggestions I updated the docblock and the test comments, and I added a test specifically for LazyCallGraph. Built and tested everything with ASAN and it passes.

Feb 13 2020, 8:07 PM · Restricted Project

Feb 9 2020

modocache added a reviewer for D72226: Add LazyCallGraph API to add function to RefSCC: hfinkel.

Now that the underlying D70927 has been committed, this diff needs a code review. I'll add @hfinkel since he left some helpful review comments on D70927.

Feb 9 2020, 6:44 PM · Restricted Project
modocache added a reviewer for D71903: [Coroutines][6/6] Clang schedules new passes: wenlei.
Feb 9 2020, 6:41 PM · Restricted Project
modocache added a reviewer for D71901: [Coroutines][4/6] New pass manager: coro-cleanup: wenlei.
Feb 9 2020, 6:41 PM · Restricted Project
modocache added a reviewer for D71900: [Coroutines][3/6] New pass manager: coro-elide: wenlei.
Feb 9 2020, 6:41 PM · Restricted Project
modocache added a reviewer for D71902: [Coroutines][5/6] Add coroutine passes to pipeline: wenlei.
Feb 9 2020, 6:41 PM · Restricted Project