Page MenuHomePhabricator

lxfind (Xun Li)
User

Projects

User does not belong to any projects.

User Details

User Since
May 3 2020, 4:37 PM (107 w, 5 d)

Recent Activity

Nov 3 2021

lxfind accepted D108696: [Coroutines] [Frontend] Lookup in std namespace first.

I also agree that we should try to keep the compiler simple and not support the complicated case.
It should be fairly straightforward for a codebase to update fully to use std instead of std::experimental (we have a large coroutine codebase as well).
Given that everyone is mostly supportive, I will accept the change.

Nov 3 2021, 7:49 PM · Restricted Project, Restricted Project, Restricted Project

Oct 21 2021

lxfind accepted D112216: [Coroutines] Ignore partial lifetime markers refer of an alloca .

Thank you for fixing this!

Oct 21 2021, 8:27 AM · Restricted Project

Sep 2 2021

lxfind committed rG2cf30c4769a5: [Coroutines] Only run verifyFunction in debug mode (authored by lxfind).
[Coroutines] Only run verifyFunction in debug mode
Sep 2 2021, 5:35 PM
lxfind closed D109198: [Coroutines] Only run verifyFunction in debug mode.
Sep 2 2021, 5:35 PM · Restricted Project
lxfind accepted D108696: [Coroutines] [Frontend] Lookup in std namespace first.

LGTM. Thanks!

Sep 2 2021, 2:34 PM · Restricted Project, Restricted Project, Restricted Project
lxfind requested review of D109198: [Coroutines] Only run verifyFunction in debug mode.
Sep 2 2021, 2:34 PM · Restricted Project

Aug 26 2021

lxfind added a comment to D108697: [libc++] Remove <experimental/coroutine>.

The warning seems redundant to the one in D108696?

Aug 26 2021, 5:24 PM · Restricted Project, Restricted Project, Restricted Project

Aug 25 2021

lxfind added a comment to D108697: [libc++] Remove <experimental/coroutine>.

In D108696, the clang would lookup in std and std::experimental namespace. And this patch would only be checked in after D108696 get checked in. So I think it wouldn't affect users much.

Well, but if user code has "#include <experimental/coroutine>", would it still compile?

Aug 25 2021, 8:35 PM · Restricted Project, Restricted Project, Restricted Project
lxfind added a comment to D108697: [libc++] Remove <experimental/coroutine>.

My guess is you might need to keep coro in both std and std::experimental for a while, to allow smooth transition.

Aug 25 2021, 9:25 AM · Restricted Project, Restricted Project, Restricted Project

Aug 24 2021

lxfind added a comment to D108615: [Coroutines] [libcxx] Move coroutine component out of experimental namespace.

I am not familiar with the process of when to move something out of experimental, but I do wonder how this is normally done so that people who uses coroutines can have a smooth migration?
I assume that this is going to be a breaking change that existing code using coroutine will need to be updated and no longer compatible with old versions.

Aug 24 2021, 9:13 AM · Restricted Project, Restricted Project

Aug 23 2021

lxfind added a comment to D108277: [Coroutines] Mark coroutine as nounwind if all the calls outside the try-catch wouldn't throw..

Thanks for working on this!
A few questions:

  • Is this primarily an optimization, or does it also fix a bug?
  • Does LLVM mid-end not have a nothrow attribute analysis pass? Doing this manually in the front-end seems a bit fragile (concerned about missing something)
  • Do we expect those isXXXNoThrow methods to be called else where? If not, is there a reason we choose to put them in 6 separate functions, instead of one function?
Aug 23 2021, 9:36 AM

Aug 3 2021

lxfind added a comment to D105877: [Coroutines] Run coroutine passes by default.

I noticed that this change had a measurable impact on O0 memory usage, which I wouldn't have expected (https://llvm-compile-time-tracker.com/compare.php?from=0f9e6451a836886f39137818c4f0cfd69ae31e62&to=8a1727ba51d262365b0d9fe10fef7e50da7022cd&stat=max-rss). Any idea what could cause it? Some additional analysis results hanging around?

Aug 3 2021, 1:21 PM · Restricted Project, Restricted Project

Aug 2 2021

lxfind added a comment to D43477: [CFG] [analyzer] Add MaterializeTemporaryExpr into the construction context..

Hi! I have a question regarding the implementation of "VisitMaterializeTemporaryExpr". Specifically, I wonder if we should skip visiting the children? Would't visiting the children of MaterializeTemporaryExpr cause the same expression to be visited twice?

Aug 2 2021, 10:21 PM

Jul 30 2021

lxfind accepted D107155: [clang][deps] Substitute clang-scan-deps executable in lit tests.

Thank you!

Jul 30 2021, 8:10 AM · Restricted Project

Jul 29 2021

lxfind added a comment to D107097: [llvm-profgen] Support perf script without parsing MMap events.

Having both "--show-mmap-events --ignore-mmap-events" seems a bit counter-intuitive.
If we already preprocessed the mmap events, could we also assume that we have the right pids and hence don't need --show-mmap-events?

Jul 29 2021, 12:58 PM · Restricted Project

Jul 26 2021

lxfind accepted D105606: [Coroutine] Record the elided coroutines .
Jul 26 2021, 9:42 PM · Restricted Project
lxfind added inline comments to D105606: [Coroutine] Record the elided coroutines .
Jul 26 2021, 7:24 PM · Restricted Project
lxfind added a comment to D105606: [Coroutine] Record the elided coroutines .

gentle ping~

fix lint?

I am not sure if I understand your point correctly. The intent of the diff is going to record the coroutines got elided. There are options to dump informations to specific file like info-output-file in Timer.cpp. So I guess we can do similar things in coroutine.

Jul 26 2021, 9:47 AM · Restricted Project

Jul 25 2021

lxfind added a comment to D105606: [Coroutine] Record the elided coroutines .

gentle ping~

Jul 25 2021, 10:53 PM · Restricted Project

Jul 19 2021

lxfind added a comment to D103593: [Coroutine] Sink lifetime markers after switch of suspend blocks to avoid disturbing must tail calls.

I am OK with this. You may want to move those part of the checking code to a separate function for readability. (since this check is independent from lifetime sinking, we could also consider doing the check earlier for all corosuspends too.

Jul 19 2021, 9:16 AM · Restricted Project

Jul 14 2021

lxfind accepted D105877: [Coroutines] Run coroutine passes by default.

LGTM

Jul 14 2021, 9:53 PM · Restricted Project, Restricted Project

Jul 13 2021

lxfind added inline comments to D105877: [Coroutines] Run coroutine passes by default.
Jul 13 2021, 8:49 AM · Restricted Project, Restricted Project

Jun 30 2021

lxfind committed rG822b92aae439: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after… (authored by lxfind).
[Coroutines] Add the newly generated SCCs back to the CGSCC work queue after…
Jun 30 2021, 11:38 AM
lxfind closed D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened.
Jun 30 2021, 11:38 AM · Restricted Project, Restricted Project
lxfind added inline comments to D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened.
Jun 30 2021, 10:17 AM · Restricted Project, Restricted Project
lxfind updated the diff for D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened.

fix warning

Jun 30 2021, 9:52 AM · Restricted Project, Restricted Project
lxfind added inline comments to D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened.
Jun 30 2021, 9:52 AM · Restricted Project, Restricted Project

Jun 29 2021

lxfind updated the diff for D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened.

Put the post-split ramp function back to the CGSCC worklist

Jun 29 2021, 8:25 PM · Restricted Project, Restricted Project
lxfind added a comment to D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened.

If coroutine ramp function couldn't get inlined, it would disable coroutine elide optimization. Could you elaborate more on why do you want to do that?

Jun 29 2021, 8:19 PM · Restricted Project, Restricted Project
lxfind accepted D105095: [Coroutine] Add statistics for the number of elided coroutine.
Jun 29 2021, 8:13 PM · Restricted Project
lxfind added a comment to D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened.

this will run the function simplification pipeline twice on every single function when coroutines are enabled, I don't think that's the intention

I thought the intention was to do all the the re-adding of SCCs inside CoroSplit.cpp, including the SCC with the function that was split

Jun 29 2021, 8:12 PM · Restricted Project, Restricted Project
lxfind added a comment to D105095: [Coroutine] Add statistics for the number of elided coroutine.

Cool! fix test failure?

Jun 29 2021, 8:48 AM · Restricted Project
lxfind added a comment to D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened.

note that we don't really need to run Inliner again on the ramp function after split

This isn't accurate. The inline may run again for ramp function after split and it's required by coro elide.

If there is an inlining opportunity, it should have happened pre-split, right? Is there any reason it didn't happen pre-split but only post-split?

It seems like that we don't need the attribute CORO_PRESPLIT_ATTR any more, do we? If yes, I think we should remove them.

It's still needed by the legacy pass manager. I don't want to break that yet.

Jun 29 2021, 8:47 AM · Restricted Project, Restricted Project

Jun 28 2021

lxfind retitled D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened from [RFC][Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened to [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened.
Jun 28 2021, 9:25 PM · Restricted Project, Restricted Project
lxfind updated the diff for D95807: [Coroutines] Add the newly generated SCCs back to the CGSCC work queue after CoroSplit actually happened.

After removing the legacy test command, I was finally able to update this patch. It's now ready for review. I will update the decription to reflect to the latest changes

Jun 28 2021, 9:22 PM · Restricted Project, Restricted Project
lxfind committed rG31eb696fc4cd: [Coroutines] Remove CoroElide from O0 pipeline (authored by lxfind).
[Coroutines] Remove CoroElide from O0 pipeline
Jun 28 2021, 7:29 PM
lxfind closed D105066: [Coroutines] Remove CoroElide from O0 pipeline.
Jun 28 2021, 7:28 PM · Restricted Project, Restricted Project
lxfind added a comment to D105066: [Coroutines] Remove CoroElide from O0 pipeline.

Yeah, but it may be inlined after splitting, which could trigger coro elide.

In O0, there is no inliner pass (after CoroSplit), so inlining should never happen.

Jun 28 2021, 7:19 PM · Restricted Project, Restricted Project
lxfind added a comment to D105066: [Coroutines] Remove CoroElide from O0 pipeline.

On O0, it is possible to inline if the user marked the function with always_inline.
Since CoroElide is kind of optimization, it should be OK to skip in O0.
Out of curiosity, what's the reason that you want to remove it?

Jun 28 2021, 6:57 PM · Restricted Project, Restricted Project
lxfind updated the diff for D105066: [Coroutines] Remove CoroElide from O0 pipeline.

update tests

Jun 28 2021, 5:05 PM · Restricted Project, Restricted Project
lxfind requested review of D105066: [Coroutines] Remove CoroElide from O0 pipeline.
Jun 28 2021, 4:12 PM · Restricted Project, Restricted Project

Jun 26 2021

lxfind added a comment to D103593: [Coroutine] Sink lifetime markers after switch of suspend blocks to avoid disturbing must tail calls.

By the way, while you are at it, I was looking at some of the sink lifetime tests, and I was confused by this test:
https://github.com/llvm/llvm-project/blob/main/llvm/test/Transforms/Coroutines/coro-split-sink-lifetime-02.ll
It's supposed to test that we can sink the lifetime (and hence will not put the alloca on the frame). But looks like we are not able to sink it and the test is testing that it will be on the frame?

Jun 26 2021, 11:52 AM · Restricted Project

Jun 25 2021

lxfind added a reviewer for D104937: [Coroutines] Define __coro_frame_ty in function scope: aprantl.
Jun 25 2021, 12:00 PM · Restricted Project
lxfind committed rGb7f24923a302: [Coroutines] Remove all legacy test command (authored by lxfind).
[Coroutines] Remove all legacy test command
Jun 25 2021, 9:47 AM
lxfind closed D104895: [Coroutines] Remove all legacy test command.
Jun 25 2021, 9:47 AM · Restricted Project
lxfind updated the diff for D104895: [Coroutines] Remove all legacy test command.

rebase

Jun 25 2021, 9:45 AM · Restricted Project

Jun 24 2021

lxfind added a reviewer for D104895: [Coroutines] Remove all legacy test command: aeubanks.
Jun 24 2021, 11:07 PM · Restricted Project
lxfind added a comment to D104895: [Coroutines] Remove all legacy test command.

Thanks for working on this!
If the two commands behaves the same, I prefer the old styles since it looks simpler.

I believe the plan is to have all tests to be in -passes style with new pm (I.e -passes is the preferred style)

Jun 24 2021, 11:06 PM · Restricted Project
lxfind requested review of D104895: [Coroutines] Remove all legacy test command.
Jun 24 2021, 9:42 PM · Restricted Project

Jun 23 2021

lxfind added a comment to D103593: [Coroutine] Sink lifetime markers after switch of suspend blocks to avoid disturbing must tail calls.

I guess I am fine with solution 1 and see how far we can go with it.
In the long run, we should look at cases where this optimization (sinkLifetimeStartMarkers) is effective and figure out why the lifetime start markers are emitted early, so that we could improve the lifetime marker creation in the first place. Overall it still feels a bit strange to move lifetime markers around (a sign that the front-end didn't do the right thing)

Jun 23 2021, 4:28 PM · Restricted Project
lxfind committed rGf09ec01f1fbb: [SjLj] Insert UnregisterFn before musttail call (authored by lxfind).
[SjLj] Insert UnregisterFn before musttail call
Jun 23 2021, 3:34 PM
lxfind closed D104807: [SjLj] Insert UnregisterFn before musttail call.
Jun 23 2021, 3:34 PM · Restricted Project
lxfind reopened D104807: [SjLj] Insert UnregisterFn before musttail call.
Jun 23 2021, 3:32 PM · Restricted Project
lxfind added a reverting change for rGf36703ada3dc: [SjLj] Insert UnregisterFn before musttail call: rGf8c84da23bc9: Revert "[SjLj] Insert UnregisterFn before musttail call".
Jun 23 2021, 3:31 PM
lxfind committed rGf8c84da23bc9: Revert "[SjLj] Insert UnregisterFn before musttail call" (authored by lxfind).
Revert "[SjLj] Insert UnregisterFn before musttail call"
Jun 23 2021, 3:31 PM
lxfind added a reverting change for D104807: [SjLj] Insert UnregisterFn before musttail call: rGf8c84da23bc9: Revert "[SjLj] Insert UnregisterFn before musttail call".
Jun 23 2021, 3:31 PM · Restricted Project
lxfind committed rGf36703ada3dc: [SjLj] Insert UnregisterFn before musttail call (authored by lxfind).
[SjLj] Insert UnregisterFn before musttail call
Jun 23 2021, 2:30 PM
lxfind closed D104807: [SjLj] Insert UnregisterFn before musttail call.
Jun 23 2021, 2:30 PM · Restricted Project
lxfind added a comment to D104807: [SjLj] Insert UnregisterFn before musttail call.

What would an IR change look like to enforce this?

Jun 23 2021, 12:53 PM · Restricted Project
lxfind added a comment to D104807: [SjLj] Insert UnregisterFn before musttail call.

The change looks good. But...

  • I wonder how many more times we'll have to deal with this.. It indeed seems hard to enforce without IR change :(
  • Are we hitting SJLJ path? I thought there's no usage of that for our codebase.
Jun 23 2021, 12:18 PM · Restricted Project
lxfind requested review of D104807: [SjLj] Insert UnregisterFn before musttail call.
Jun 23 2021, 11:59 AM · Restricted Project

Jun 22 2021

lxfind added a comment to D103593: [Coroutine] Sink lifetime markers after switch of suspend blocks to avoid disturbing must tail calls.

To be honest, none of the solutions seems satisfying but I couldn't propose a better one.

Jun 22 2021, 10:56 AM · Restricted Project

Jun 17 2021

lxfind committed rG3522167efd80: [Coroutine] Properly deal with byval and noalias parameters (authored by lxfind).
[Coroutine] Properly deal with byval and noalias parameters
Jun 17 2021, 7:06 PM
lxfind closed D104184: [Coroutine] Properly deal with byval and noalias parameters.
Jun 17 2021, 7:06 PM · Restricted Project
lxfind updated the diff for D104184: [Coroutine] Properly deal with byval and noalias parameters.

Add test case for noalias argument

Jun 17 2021, 9:21 AM · Restricted Project

Jun 16 2021

lxfind added inline comments to D104184: [Coroutine] Properly deal with byval and noalias parameters.
Jun 16 2021, 8:36 PM · Restricted Project
lxfind abandoned D104007: [BasicAA] Properly mark that coroutine suspension may modify parameters.
Jun 16 2021, 10:34 AM · Restricted Project
lxfind added a comment to D104007: [BasicAA] Properly mark that coroutine suspension may modify parameters.

Abandon this in favor of D104184

Jun 16 2021, 10:34 AM · Restricted Project

Jun 15 2021

lxfind added inline comments to D104184: [Coroutine] Properly deal with byval and noalias parameters.
Jun 15 2021, 8:09 AM · Restricted Project

Jun 14 2021

lxfind updated the diff for D104184: [Coroutine] Properly deal with byval and noalias parameters.

update comment

Jun 14 2021, 9:24 PM · Restricted Project
lxfind added a comment to D104184: [Coroutine] Properly deal with byval and noalias parameters.

I updated the description to reflex the summary of discussions from previous patches. Based on the discussion, we believe that this should be the right approach

Jun 14 2021, 8:06 PM · Restricted Project
lxfind added inline comments to D104184: [Coroutine] Properly deal with byval and noalias parameters.
Jun 14 2021, 5:22 PM · Restricted Project
lxfind retitled D104184: [Coroutine] Properly deal with byval and noalias parameters from [Coroutine] Put byval params' value into frame, instead of just pointer to [Coroutine] Properly deal with byval and noalias parameters.
Jun 14 2021, 5:04 PM · Restricted Project
lxfind added a comment to D104007: [BasicAA] Properly mark that coroutine suspension may modify parameters.

However latter optimization passes could very well optimize out that copy because the copy would appear to be useless without considering what coro_suspend does, which is why we are changing BasicAA to prevent such elimination.

If an argument is just a pointer without any argument attributes, BasicAA will assume coro_suspend frees the underlying allocations. Not because it has any particular knowledge of how coro_suspend works, but because that's the default assumption for any function call with unknown semantics.

BasicAA treats arguments marked byval differently because it understands the semantics of byval: it uses escape analysis to prove an arbitrary function call can't modify the contents.


I guess in some sense, it's possible to specify a world where coro_suspend frees byval allocations. But such a world doesn't really make sense. And we'd need to audit every pass that knows anything about byval semantics, so I really don't want to go down that path.

Jun 14 2021, 4:55 PM · Restricted Project
lxfind updated the diff for D104184: [Coroutine] Properly deal with byval and noalias parameters.

only check byval attribute

Jun 14 2021, 4:37 PM · Restricted Project
lxfind added a comment to D104007: [BasicAA] Properly mark that coroutine suspension may modify parameters.

From clang's perspective, what it needs to ensure is that every argument is stored in an allocation local to the callee (in other words, allocations corosplit can handle). "allocations corosplit can handle" should be either allocas, or arguments marked byval. These are pointers that don't exist in the caller, so corosplit can mess with the address in memory without worrying about code it can't see in the caller.

How do you prevent optimization passes (prior to CoroSplit) from messing up with those pointers, though?
Let's say we have an argument that's not marked with byval (due to target specifics) but is pass-by-value in C++. In Clang we could emit IR to copy the argument value to a local alloca. However latter optimization passes could very well optimize out that copy because the copy would appear to be useless without considering what coro_suspend does, which is why we are changing BasicAA to prevent such elimination.
Or are you suggesting we need to modify Clang such that every pass-by-value argument in C++ must be marked with byval?

Jun 14 2021, 11:32 AM · Restricted Project
lxfind added a comment to D104007: [BasicAA] Properly mark that coroutine suspension may modify parameters.

It seems that there is no stable contract to tell whether an argument is byval, and it depends on the target.
For example: https://godbolt.org/z/YcrYPbMdc
So we won't be able to fix this just in CoroSplit pass. Either we change the front-end to force all byval arguments to have byval attribute (not sure if that's possible), or we may have to conservatively go with this patch.

I'm not sure what you mean. An LLVM IR argument either has the byval attribute, or it doesn't. If it does, the allocation belongs to the callee. If it doesn't, the pointer refers to an allocation constructed in the caller. There isn't any other sense in which an argument can be "byval".

A given C++ function can have a bunch of variations in the LLVM IR signature, depending on the target, but that's not relevant here. Once clang is finished emitting IR, the C++ type signature becomes irrelevant.

Unfortunately what we really care about here is whether an argument is by-value from C++ perspective, not just whether it has a byval attribute. Let me try to explain the problem top-down and see if I got it wrong somewhere:

  1. In C++, when we call a function with a parameter passed by value, such as foo(std::move(a)), the passed value is typically a temporary value allocated at caller, and only the pointer is passed to the callee. When callee returns, the caller will be able to deallocate that temporary value's memory (in LLVM IR terms, there will be a lifetime.end marking the temporary value after the callee returns).
  2. If the callee is a normal routine (non-coroutine), the callee don't need to worry about the lifetime of the parameter because it will stay alive through the entire life cycle of the callee.
  3. However if the callee is a coroutine, the situation is different because the coroutine can return in the middle of the function and latter resume. As soon as the coroutine returns for the first time (i.e. suspended the first time), the temporary value (the pass-by-value parameter) would die in the caller. But if the callee needs to use that argument after coroutine resumption, we need to make sure that the argument value will be put on the coroutine frame (similar to allocas), so that latter access won't be accessing invalid memory.
  4. However there is one major difference between a parameter and an alloca: for a parameter that is a pointer type, we need to know whether we need to copy the value that the pointer points to to the coroutine frame, or just the pointer it self. For instance, you could call a coroutine by passing a direct pointer, and in that case we should only need to copy the pointer to the frame. But for the C++ pass-by-value arguments, we need to copy the value that the pointer points to to the frame. The question is then, during CoroSplit (the pass where we put things to the frame), are we able to tell, given an argument pointer, whether we need to copy the value or just the pointer to the frame? Knowing that a C++ pass-by-value argument doesn't necessarily end up with an argument with a byval attribute, we know that it is not possible to tell, hence we cannot deal with this properly only in CoroSplit pass.

That this sound right?

Jun 14 2021, 9:00 AM · Restricted Project

Jun 13 2021

lxfind added a comment to D104007: [BasicAA] Properly mark that coroutine suspension may modify parameters.

Do you know if there is a stable contract in IR to tell whether an argument is in fact a callee-owned memory?

Argument::hasPassPointeeByValueCopyAttr()

Sorry, that's not right. The question you want to answer here is whether the pointer in the caller points to different memory from the pointer in the callee. The only attribute that causes that is byval, and it's very unlikely we'll ever add any others.

inalloca/preallocated memory is part of the callee stack frame, at a low level. But it's actually allocated in the caller, so from an alias-analysis perspective, it doesn't count as a local allocation like byval would.

Jun 13 2021, 8:58 AM · Restricted Project
lxfind added a comment to D104184: [Coroutine] Properly deal with byval and noalias parameters.

I just realized that this is now getting very similar to D101980, and we are facing the same problem of not always be able to tell whether a param is byval in the midend. Let me continue the discussion on the BasicAA patch.

Jun 13 2021, 8:54 AM · Restricted Project

Jun 12 2021

lxfind committed rGfae7debadcea: [CHR] Don't run ControlHeightReduction if any BB has address taken (authored by lxfind).
[CHR] Don't run ControlHeightReduction if any BB has address taken
Jun 12 2021, 10:30 AM
lxfind closed D103867: [CHR] Don't run ControlHeightReduction if any BB has address taken.
Jun 12 2021, 10:30 AM · Restricted Project
lxfind updated the summary of D103867: [CHR] Don't run ControlHeightReduction if any BB has address taken.
Jun 12 2021, 10:29 AM · Restricted Project
lxfind requested review of D104184: [Coroutine] Properly deal with byval and noalias parameters.
Jun 12 2021, 10:12 AM · Restricted Project

Jun 11 2021

lxfind added a comment to D104007: [BasicAA] Properly mark that coroutine suspension may modify parameters.

Do you know if there is a stable contract in IR to tell whether an argument is in fact a callee-owned memory?

Argument::hasPassPointeeByValueCopyAttr()

Sorry, that's not right. The question you want to answer here is whether the pointer in the caller points to different memory from the pointer in the callee. The only attribute that causes that is byval, and it's very unlikely we'll ever add any others.

inalloca/preallocated memory is part of the callee stack frame, at a low level. But it's actually allocated in the caller, so from an alias-analysis perspective, it doesn't count as a local allocation like byval would.

hmm sounds like hasPassPointeeByValueCopyAttr() is actually what I need. In CoroSplit, when deciding what to put on the coroutine frame, for arguments, I will need to decide whether it's enough to just put the pointer on the frame, or if I need to copy the content of the pointer (much like allocas) to the frame. Sounds like for inalloca and preallocated memory I still want to copy the content to the frame, not just the pointer.

Jun 11 2021, 5:23 PM · Restricted Project
lxfind added a comment to D103867: [CHR] Don't run ControlHeightReduction if any BB has address taken.

Poking around it looks like this should be handled for inlining, not sure why the previous case we ran into wasn't covered by that code below from CallAnalyzer::analyze()

// Disallow inlining a blockaddress with uses other than strictly callbr.
// A blockaddress only has defined behavior for an indirect branch in the
// same function, and we do not currently support inlining indirect
// branches.  But, the inliner may not see an indirect branch that ends up
// being dead code at a particular call site. If the blockaddress escapes
// the function, e.g., via a global variable, inlining may lead to an
// invalid cross-function reference.
// FIXME: pr/39560: continue relaxing this overt restriction.
if (BB->hasAddressTaken())
  for (User *U : BlockAddress::get(&*BB)->users())
    if (!isa<CallBrInst>(*U))
      return InlineResult::failure("blockaddress used outside of callbr");

LGTM, thanks for the fix.

Jun 11 2021, 5:20 PM · Restricted Project
lxfind added a comment to D104007: [BasicAA] Properly mark that coroutine suspension may modify parameters.

If you fix the clang code so it isn't generating these extra useless allocas, the problem with byval should become obvious: even without any optimizations, we end up with a use-after-free. corosplit isn't preserving memory that's supposed to be part of the callee frame.

Jun 11 2021, 8:21 AM · Restricted Project

Jun 10 2021

lxfind added a comment to D104007: [BasicAA] Properly mark that coroutine suspension may modify parameters.

This doesn't seem right. A byval argument is essentially a local variable. We should treat it the same way as we would an alloca. If it's live across the first suspend point, it should become part of the coroutine frame.

It will become part of the coroutine frame, by copying it through memcpy. That's actually what's the first memcpy for in the test case.

I don't understand. As far as I can tell, the destination of both memcpys in the testcase are allocas.

OK, so the way coroutine codegen works is to first copy all parameters to local alloca, and then if any of those allocas need to live through coroutine suspension, the alloca will be put on the coroutine frame during the CoroSplit pass.

Jun 10 2021, 10:37 PM · Restricted Project
lxfind added a comment to D104007: [BasicAA] Properly mark that coroutine suspension may modify parameters.

This doesn't seem right. A byval argument is essentially a local variable. We should treat it the same way as we would an alloca. If it's live across the first suspend point, it should become part of the coroutine frame.

Jun 10 2021, 9:38 PM · Restricted Project
lxfind added a comment to D104007: [BasicAA] Properly mark that coroutine suspension may modify parameters.

This doesn't seem right. A byval argument is essentially a local variable. We should treat it the same way as we would an alloca. If it's live across the first suspend point, it should become part of the coroutine frame.

Jun 10 2021, 9:29 PM · Restricted Project
lxfind updated the diff for D104007: [BasicAA] Properly mark that coroutine suspension may modify parameters.

address comment

Jun 10 2021, 4:45 PM · Restricted Project
lxfind abandoned D102465: [Coroutines] Mark every parameter.
Jun 10 2021, 1:27 PM · Restricted Project, Restricted Project

Jun 9 2021

lxfind added a comment to D101510: Do not merge memcpy if the first source is a parameter of coroutine function.

I found a more proper fix in D104007.

Jun 9 2021, 7:49 PM · Restricted Project
lxfind requested review of D104007: [BasicAA] Properly mark that coroutine suspension may modify parameters.
Jun 9 2021, 7:48 PM · Restricted Project
lxfind abandoned D101510: Do not merge memcpy if the first source is a parameter of coroutine function.
Jun 9 2021, 7:41 PM · Restricted Project

Jun 8 2021

lxfind updated the diff for D103867: [CHR] Don't run ControlHeightReduction if any BB has address taken.

more test

Jun 8 2021, 10:02 PM · Restricted Project
lxfind updated the diff for D103867: [CHR] Don't run ControlHeightReduction if any BB has address taken.

Only skip regions that contain BBs with address taken

Jun 8 2021, 10:01 PM · Restricted Project
lxfind updated the diff for D103867: [CHR] Don't run ControlHeightReduction if any BB has address taken.

Add test case

Jun 8 2021, 5:46 PM · Restricted Project
lxfind added a comment to D103867: [CHR] Don't run ControlHeightReduction if any BB has address taken.

do all passes that clone BBs have to worry about this?

I think so, as long as the cloned BB is in the same module as the original one, we need to worry about this (there are legit cloning in some tools such as llvm-extract, llvm-reduce..)

Jun 8 2021, 3:34 PM · Restricted Project
lxfind added inline comments to D103593: [Coroutine] Sink lifetime markers after switch of suspend blocks to avoid disturbing must tail calls.
Jun 8 2021, 9:27 AM · Restricted Project

Jun 7 2021

lxfind updated the summary of D103867: [CHR] Don't run ControlHeightReduction if any BB has address taken.
Jun 7 2021, 10:26 PM · Restricted Project