[PATCH] D131329: [SDAG] Add `getCALLSEQ_END` overload taking `uint64_t`s

[Sergei Barannikov via Phabricator via llvm-commits]

barannikov88 added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/SelectionDAG.h:967
 
+  SDValue getCALLSEQ_END(SDValue Chain, uint64_t Size1, uint64_t Size2,
+                         SDValue Glue, const SDLoc &DL) {
----------------
barannikov88 wrote:
> jrtc27 wrote:
> > barannikov88 wrote:
> > > jrtc27 wrote:
> > > > Size1/Size2 aren't great names; getCALLSEQ_START calls them InSize and OutSize which seems better to me.
> > > The meaning of these values is target-dependent (see the comment near CALLSEQ_START/CALLSEQ_END nodes). Giving them any specific names would suggest the opposite. "InSize" and "OutSize" are not very meaningful names anyway (what is "in" and "out" here?).
> > That full comment is:
> > ```
> >   /// CALLSEQ_START/CALLSEQ_END - These operators mark the beginning and end
> >   /// of a call sequence, and carry arbitrary information that target might
> >   /// want to know.  The first operand is a chain, the rest are specified by
> >   /// the target and not touched by the DAG optimizers.
> >   /// Targets that may use stack to pass call arguments define additional
> >   /// operands:
> >   /// - size of the call frame part that must be set up within the
> >   ///   CALLSEQ_START..CALLSEQ_END pair,
> >   /// - part of the call frame prepared prior to CALLSEQ_START.
> >   /// Both these parameters must be constants, their sum is the total call
> >   /// frame size.
> >   /// CALLSEQ_START..CALLSEQ_END pairs may not be nested.
> > ```
> > CALLSEQ_START and CALLSEQ_END are identical in this regard, so having the mismatch in naming between getCALLSEQ_START and getCALLSEQ_END is odd to me. Note that on architectures that don't use the stack for arguments (like NVPTX) these aren't even sizes; NVPTX just uses an incrementing counter that exists solely to end up in assembly comments AFAICT.
> > CALLSEQ_START and CALLSEQ_END are identical in this regard
> 
> It seems so, according to the comment. In reality, however, targets give different meaning to the second argument of the CALLSEQ_END. X86, for instance, uses it to record the "callee pop" size (i.e. the adjustment made to the stack pointer by the call).
> I understand your concern, I myself don't like vague names, but in this particular case I think making the names specific is making them misleading.
> My last justification is that the overloaded function names these values as "Op1" and "Op2".
> 
Please also note that these nodes have other uses, not related to calls. See `ExpandDYNAMIC_STACKALLOC`, for example.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131329