Page MenuHomePhabricator

modocache (Brian Gesiak)
User

Projects

User does not belong to any projects.

User Details

User Since
Feb 16 2014, 1:10 AM (320 w, 2 d)

Recent Activity

Sat, Apr 4

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

Thu, Apr 2

modocache committed rG627e01feb718: [coroutines] Don't build promise init with no args (authored by modocache).
[coroutines] Don't build promise init with no args
Thu, Apr 2, 7:00 PM
modocache closed D70555: [coroutines] Don't build promise init with no args.
Thu, Apr 2, 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!

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

LGTM, thanks!

Thu, Apr 2, 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!

Thu, Apr 2, 10:50 AM · Restricted Project

Sat, Mar 28

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

Looks great, thanks!

Sat, Mar 28, 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!

Sat, Mar 28, 5:44 PM · Restricted Project

Thu, Mar 19

modocache accepted D76345: [Coroutines] Fix PR45130.

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

Thu, Mar 19, 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.

Thu, Mar 19, 1:42 PM · Restricted Project

Wed, Mar 18

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

Awesome, thank you!

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

Tue, Mar 17

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:

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

Fri, Mar 13

modocache added reviewers for D70555: [coroutines] Don't build promise init with no args: junparser, wenlei.
Fri, Mar 13, 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.

Fri, Mar 13, 7:28 AM · Restricted Project
modocache added inline comments to D76119: [Coroutines] Insert lifetime intrinsics even O0 is used.
Fri, Mar 13, 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.

Fri, Mar 13, 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.

Fri, Mar 13, 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.

Fri, Mar 13, 5:46 AM · Restricted Project

Thu, Mar 12

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?

Thu, Mar 12, 11:23 AM · Restricted Project

Wed, Mar 11

modocache updated the summary of D76016: [ProfileData] Add option to ignore invalid regions.
Wed, Mar 11, 11:52 AM · Restricted Project
modocache created D76016: [ProfileData] Add option to ignore invalid regions.
Wed, Mar 11, 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
modocache added a reviewer for D71899: [Coroutines][2/6] New pass manager: coro-split: wenlei.
Feb 9 2020, 6:41 PM · Restricted Project

Feb 3 2020

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

Use the latest outlining interface from D72226, which was just updated based on the latest version of D70927.

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

Update based on @jdoerfert's latest version of revision D70927. Previous versions added a public member function to CallGraphUpdater, the latest versions instead add a public member function to LazyCallGraph. This update does the same, it moves the new member function, from CallGraphUpdater::registerReferredToOutlinedFunction, to LazyCallGraph::addNewFunctionIntoRefSCC.

Feb 3 2020, 8:14 PM · Restricted Project
modocache added inline comments to D70927: Introduce a CallGraph updater helper class.
Feb 3 2020, 7:18 PM · Restricted Project
modocache accepted D70927: Introduce a CallGraph updater helper class.

I mentioned in my last response that you moved the "problematic" logic into LazyCallGraph in your patch (D72226), or did you do something different? Please clarify what you want me to do that is different, or the same, to your patch. I'm happy to move logic into the LazyCallGraph class for example.

Feb 3 2020, 5:55 PM · Restricted Project
modocache requested changes to D70927: Introduce a CallGraph updater helper class.

Sorry if it's presumptuous of me to request changes, but I guess @hfinkel may agree with me here that registerOutlinedFunction ought to be moved to LazyCallGraph or one of its inner classes.

Feb 3 2020, 9:47 AM · Restricted Project

Feb 1 2020

modocache committed rGd82e993cd34a: [ADT] 'PointerUnion::is' returns 'bool' (authored by modocache).
[ADT] 'PointerUnion::is' returns 'bool'
Feb 1 2020, 1:56 PM
modocache closed D73836: [ADT] 'PointerUnion::is' returns 'bool'.
Feb 1 2020, 1:56 PM · Restricted Project
modocache added a comment to D73836: [ADT] 'PointerUnion::is' returns 'bool'.

No, my only motivation was reading the interface and doing a double-take. I thought maybe the int return type was meant to not only signify whether the pointer was to the given type, but also the index at which that type existed in the union. I had to read the body of the function to learn that this wasn't the case. A bool return type, at least in my case, would have prevented that initial confusion.

Feb 1 2020, 1:56 PM · Restricted Project