rjmccall added inline comments.
================ Comment at: clang/include/clang/CodeGen/CGFunctionInfo.h:91 bool InAllocaSRet : 1; // isInAlloca() + bool InAllocaIndirect : 1;// isInAlloca() bool IndirectByVal : 1; // isIndirect() ---------------- rnk wrote: > rjmccall wrote: > > rnk wrote: > > > rnk wrote: > > > > rjmccall wrote: > > > > > Would it be better to handle `inalloca` differently, maybe as a flag > > > > > rather than as a top-level kind? I'm concerned about gradually > > > > > duplicating a significant amount of the expressivity of other kinds. > > > > In the past, I've drafted a more than one unfinished designs for how we > > > > could remodel inalloca with tokens so that it can be per-argument > > > > instead of something that applies to all argument memory. > > > > Unfortunately, I never found the time to finish or implement one. > > > > > > > > As I was working on this patch, I was thinking to myself that this > > > > could be the moment to implement one of those designs, but it would be > > > > pretty involved. Part of the issue is that, personally, I have very > > > > little interest in improving x86_32 code quality, so a big redesign > > > > wouldn't deliver much benefit. The benefits would all be code > > > > simplifications and maintenance cost reductions, which are nice, but > > > > seem to only get me through the prototype design stage. > > > > > > > > I'll go dig up my last doc and try to share it, but for now, I think we > > > > have to suffer the extra inalloca code in this patch. > > > Here's what I wrote, with some sketches of possible LLVM IR that could > > > replace inalloca: > > > https://reviews.llvm.org/P8183 > > > > > > The basic idea is that we need a call setup instruction that forms a > > > region with the call. During CodeGen, we can look forward (potentially > > > across BBs) to determine how much argument stack memory to allocate, > > > allocate it (perhaps in pieces as we go along), and then skip the normal > > > call stack argument adjustment during regular call lowering. > > > > > > Suggestions for names better than "argmem" are welcome. > > > > > > The major complication comes from edges that exit the call setup region. > > > These could be exceptional exits or normal exits with statement > > > expressions and break, return, or goto. Along these paths we need to > > > adjust SP, or risk leaking stack memory. Today, with inalloca, I think we > > > leak stack memory. > > > In the past, I've drafted a more than one unfinished designs for how we > > > could remodel inalloca with tokens so that it can be per-argument instead > > > of something that applies to all argument memory. Unfortunately, I never > > > found the time to finish or implement one. > > > > Sorry! I think it would be great to rethink `inalloca` to avoid the > > duplication and so on, but I certainly didn't mean to suggest that we > > should do that as part of this patch. (I'll look at your proposal later.) > > I was trying to ask if it would make sense to change how `inalloca` > > arguments are represented by `ABIInfo`, so that we could e.g. build a > > normal indirect `ABIInfo` and then flag that it also needs to be written > > into an `inalloca` buffer. > I see. I think implementing that would require a greater refactoring of > ClangToLLVMArgMapping. We'd need a new place to store the inalloca struct > field index. That is currently put in a union with some other stuff, which > would no longer work. Okay. Then in the short term, I guess this is fine, and the long-term improvement is to change the `inalloca` design. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72114/new/ https://reviews.llvm.org/D72114 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
