[llvm-commits] [llvm] r46827 - MemOperands #2/2

[Chris Lattner]

On Feb 6, 2008, at 2:27 PM, Dan Gohman wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=46827&view=rev
> Log:
> Re-apply the memory operand changes, with a fix for the static
> initializer problem, a minor tweak to the way the
> DAGISelEmitter finds load/store nodes, and a renaming of the
> new PseudoSourceValue objects.

Continuing the review:

> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- llvm/trunk/lib/CodeGen/SelectionDAG/ScheduleDAG.cpp (original)
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/ScheduleDAG.cpp Wed Feb  6  
> 16:27:42 2008
> @@ -277,15 +277,27 @@
>   return N;
> }
>
> +/// CountOperands - The inputs to target nodes have any actual  
> inputs first,
> +/// followed by optional memory operands chain operand, then flag  
> operands.

Strictly speaking, a node can have at most one flag operand.  This  
comment isn't your bug, but it would be nice to fix it :)

>
> +/// Compute the number of actual operands that  will go into the  
> machine istr.

istr -> instr or instruction?  Also, double space before 'will'.

>
> unsigned ScheduleDAG::CountOperands(SDNode *Node) {
>   unsigned N = Node->getNumOperands();
>   while (N && Node->getOperand(N - 1).getValueType() == MVT::Flag)
>     --N;
>   if (N && Node->getOperand(N - 1).getValueType() == MVT::Other)
>     --N; // Ignore chain if it exists.
> +  while (N && MemOperandSDNode::classof(Node->getOperand(N - 1).Val))

Instead of calling classof, please use:

  N && isa<MemOperandSDNode>(...)

>
> +    --N; // Ignore MemOperand nodes
> +  return N;
> +}
> +
> +/// CountMemOperands - Find the index of the last MemOperandSDNode  
> operand
> +unsigned ScheduleDAG::CountMemOperands(SDNode *Node) {

Please rename this method.  The name implies that it returns the  
number of mem operands, not the index of the last one.

> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp (original)
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAG.cpp Wed Feb  6  
> 16:27:42 2008
> @@ -3503,6 +3535,26 @@
>   TheGlobal = const_cast<GlobalValue*>(GA);
> }
>
> +/// getMemOperand - Return a MemOperand object describing the memory
> +/// reference performed by this load or store.
> +MemOperand LSBaseSDNode::getMemOperand() const {
> +  int Size = (MVT::getSizeInBits(getMemoryVT()) + 7) >> 3;
> +  int Flags =
> +    getOpcode() == ISD::LOAD ? MemOperand::MOLoad :  
> MemOperand::MOStore;
> +  if (IsVolatile) Flags |= MemOperand::MOVolatile;
> +
> +  // Check if the load references a frame index, and does not have
> +  // an SV attached.
> +  const FrameIndexSDNode *FI =
> +    dyn_cast<const FrameIndexSDNode>(getBasePtr().Val);
> +  if (!getSrcValue() && FI)
> +    return MemOperand(&PseudoSourceValue::getFixedStack(), Flags,
> +                      FI->getIndex(), Size, Alignment);
> +  else
> +    return MemOperand(getSrcValue(), Flags,
> +                      getSrcValueOffset(), Size, Alignment);

This logic seems correct, but would be more clear (at least to me) if  
written as:

if (getSrcValue() || !FI)
> +    return MemOperand(getSrcValue(), Flags,
> +                      getSrcValueOffset(), Size, Alignment);
else
> +  if (!getSrcValue() && FI)
> +    return MemOperand(&PseudoSourceValue::getFixedStack(), Flags,
> +                      FI->getIndex(), Size, Alignment);


It would be even easier if LSBaseSDNode just contained a memoperand to  
return by const reference though :)


> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================

> --- llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp (original)
> +++ llvm/trunk/lib/Target/PowerPC/PPCISelLowering.cpp Wed Feb  6  
> 16:27:42 2008
> @@ -2199,9 +2203,11 @@
>                                 Op.getOperand(0));
>
>   // STD the extended value into the stack slot.
> +  MemOperand MO(&PseudoSourceValue::getFixedStack(),
> +                MemOperand::MOStore, FrameIdx, 8, 8);

Ah, this is interesting.  I had to go look at the header file to make  
sure this is correct: wouldn't it make sense for the offset/index to  
be passed after the Value*?  I would expect to see something like:

> +  MemOperand MO(&PseudoSourceValue::getFixedStack(), FrameIdx,
> +                MemOperand::MOStore, 8, 8);

instead of splitting the two.  If MemOperand stored an MVT instead of  
a size, it would make it a bit more clear what was going on, because  
the magic constants would be reduced:

> +  MemOperand MO(&PseudoSourceValue::getFixedStack(), FrameIdx,
> +                MemOperand::MOStore, MVT::i64, 8);

etc.


> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/utils/TableGen/DAGISelEmitter.cpp (original)
> +++ llvm/trunk/utils/TableGen/DAGISelEmitter.cpp Wed Feb  6 16:27:42  
> 2008
> @@ -313,6 +313,12 @@
>   std::vector<std::pair<std::string, std::string> > OrigChains;
>   std::set<std::string> Duplicates;
>
> +  /// LSI - Load/Store information.
> +  /// Save loads/stores matched by a pattern, and generate a  
> MemOperandSDNode
> +  /// for each memory access. This facilitates the use of  
> AliasAnalysis in
> +  /// the backend.
> +  std::vector<std::string> LSI;
> +
>   /// GeneratedCode - This is the buffer that we emit code to.  The  
> first int
>   /// indicates whether this is an exit predicate (something that  
> should be
>   /// tested, and if true, the match fails) [when 1], or normal code  
> to emit
> @@ -373,6 +379,16 @@
>   void EmitMatchCode(TreePatternNode *N, TreePatternNode *P,
>                      const std::string &RootName, const std::string  
> &ChainSuffix,
>                      bool &FoundChain) {
> +
> +    // Save loads/stores matched by a pattern.
> +    if (!N->isLeaf() && N->getName().empty()) {
> +      std::string EnumName = N->getOperator()- 
> >getValueAsString("Opcode");
> +      if (EnumName == "ISD::LOAD" ||
> +          EnumName == "ISD::STORE") {
> +        LSI.push_back(RootName);
> +      }
> +    }
> +
>     bool isRoot = (P == NULL);
>     // Emit instruction predicates. Each predicate is just a string  
> for now.
>     if (isRoot) {
> @@ -944,6 +960,18 @@
>         }
>       }
>
> +      // Generate MemOperandSDNodes nodes for each memory accesses  
> covered by this
> +      // pattern.
> +      if (isRoot) {
> +        std::vector<std::string>::const_iterator mi, mie;
> +        for (mi = LSI.begin(), mie = LSI.end(); mi != mie; ++mi) {
> +          emitCode("SDOperand LSI_" + *mi + " = "
> +                   "CurDAG->getMemOperand(cast<LSBaseSDNode>(" +
> +                   *mi + ")->getMemOperand());");
> +          AllOps.push_back("LSI_" + *mi);
> +        }
> +      }
> +

Evan, please review this tblgen change.

-Chris