[PATCH] D101980: [RFC] [Coroutines] Put the contents of byval argument to the frame

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added a comment.

In D101980#2751187 <https://reviews.llvm.org/D101980#2751187>, @rjmccall wrote:

> Implicitly by reference, i.e. the argument type is not a reference type but the ABI just passes a pointer behind the scenes.

Does it mean there are arguments who should be passed by value but are passed by reference actually? If it is true, we are in trouble.

> byval in LLVM IR has a very specific meaning: it means the argument is passed in the stack argument area. On the caller side there's an implicit copy from the argument pointer into the argument area, and on the callee side the parameter pointer value is directly bound to the position in the argument area. A lot of targets never use it for anything, because it's not considered beneficial for small arguments (like integers and small structs), and those targets pass large aggregates indirectly (as discussed above).

I see, since the usage of `byval` is very limited. So the problems we can solve by using `byval` is very limited.

> You can't assume that move construction is equivalent to a memcpy. That's just not semantically correct. Only the frontend has this information.

Yeah, I need to investigate more. The problem here I found is the destruction process. If there isn't destruction, I think it is fun to use memcpy here. But with destruction, we may be in trouble:

  struct. A {
      void *data_ptr;
      A(A&&);
      ~A() {
          delete data_ptr;
      }
  };

If we memcpy A into the frame, then it is possible that the `data_ptr` is already deleted when we need to use the `data_ptr` in the resumed coroutine. The model is kinda complex in my head. I need to check the c++ standard and the LLVM implementation more to make a further decision.

> I am saying that there is a risk of miscompilation if LLVM applies assumptions that are valid in normal functions but aren't valid in unlowered coroutines. Probably the right solution is just to teach LLVM that those assumptions aren't valid in unlowered coroutines. You should probably bring it up on llvm-dev.

We may need to insert many guard codes in other passes if we want to teach LLVM the rules about coroutine. From my minds, the complexity of the codes becomes very high if we did so. I am not sure if it is the only right way to solve it. I would do more investigation. But I would be happy to bring it up to the llvm-dev first.

By the way, do you @rjmccall think this patch is acceptable when it doesn't fix all the problems? If yes, I would try to handle the case about `inalloca` and `preallocated`. If no, I would mark this with `plan changes`. From my point of view, I think we can work with it now at least it solves part of the problems.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D101980/new/

https://reviews.llvm.org/D101980