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

2008-02-12 Thread Evan Cheng 

On Feb 12, 2008, at 10:38 AM, Dan Gohman wrote:


 On Feb 12, 2008, at 12:12 AM, Evan Cheng wrote:

 +
 +// 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);
 +  }
 +}

 This doesn't seem safe. What if the pattern involves target  
 specific load / store nodes? Perhaps you can add a node property,  
 e.g. SDNPHasMemOp, to tell tblgen which operands would add memory  
 operands to the resulting target node?

 The problem is that this code really needs an LSBaseSDNode. A target- 
 specific
 load/store won't have that. I'm looking for better ways to solve  
 this. One way is
 to require all target-specific load/store instructions to have  
 patterns, and that
 they must use ld/st/ist to describe their memory references. I'm not  
 familiar enough
 with all the targets yet to know if that's feasible.

Ok, then please just add a SDNPHasMemOp and mark ld, st, and ist with  
it. tblgen should look for that property instead of EnumName. I don't  
think there are target specific load / store nodes that might be  
impacted at this tme.

Evan



 Dan


___
llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

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

2008-02-12 Thread Dan Gohman 

On Feb 12, 2008, at 12:12 AM, Evan Cheng wrote:

 +
 +// 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);
 +  }
 +}

 This doesn't seem safe. What if the pattern involves target specific  
 load / store nodes? Perhaps you can add a node property, e.g.  
 SDNPHasMemOp, to tell tblgen which operands would add memory  
 operands to the resulting target node?

The problem is that this code really needs an LSBaseSDNode. A target- 
specific
load/store won't have that. I'm looking for better ways to solve this.  
One way is
to require all target-specific load/store instructions to have  
patterns, and that
they must use ld/st/ist to describe their memory references. I'm not  
familiar enough
with all the targets yet to know if that's feasible.

Dan

___
llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

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

2008-02-10 Thread Chris Lattner 
On Feb 6, 2008, at 2:27 PM, Dan Gohman wrote:
 URL: http://llvm.org/viewvc/llvm-project?rev=46827view=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  isaMemOperandSDNode(...)


 +--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_castGlobalValue*(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_castconst 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::vectorstd::pairstd::string, std::string  OrigChains;
   std::setstd::string Duplicates;

 +  /// LSI - Load/Store information.
 +  /// Save