[PATCH] D72114: [MS] Overhaul how clang passes overaligned args on x86_32

[John McCall via Phabricator via cfe-commits]

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