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 _______________________________________________ llvm-commits mailing list llvm-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits