[llvm-commits] [llvm] r46827 - memoperands #1
Chris Lattner
clattner at apple.com
Sun Feb 10 11:56:28 PST 2008
On Feb 6, 2008, at 2:27 PM, Dan Gohman wrote:
> URL: http://llvm.org/viewvc/llvm-project?rev=46827&view=rev
> Log:
> Create a new class, MemOperand, for describing memory references
> in the backend. Introduce a new SDNode type, MemOperandSDNode, for
> holding a MemOperand in the SelectionDAG IR, and add a MemOperand
> list to MachineInstr, and code to manage them. Remove the offset
> field from SrcValueSDNode; uses of SrcValueSDNode that were using
> it are all all using MemOperandSDNode now.
>
> Also, begin updating some getLoad and getStore calls to use the
> PseudoSourceValue objects.
>
> Most of this was written by Florian Brander, some
> reorganization and updating to TOT by me.
>
> 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.
This is very nice work guys. Some thoughts:
class MemOperand {
Should this be named MachineMemOperand, or something like that, for
consistency?
unsigned int Flags;
int Offset;
int Size;
unsigned int Alignment;
Is 32 bits sufficient for offset information? Are there any targets
that can do reg+largeoffset?
If you store Alignment in power-of-two form, you can make it be a
short, and then pack flags+alignment into the same word.
Instead of Size here, would it make sense to store an MVT? That would
seem to capture strictly more information, thought I'm not sure if
it's directly useful right now.
Is the Value* always required to have llvm::PointerType if nonnull?
If so, it would be useful to add a comment stating that. When we have
more support for alternate address spaces in the backend, this could
be a useful invariant to have.
In MachineInstr, is there any semantics associated with the ordering
of memoperands? Are there any current targets that have instructions
with multiple memoperands?
> Modified: llvm/trunk/include/llvm/CodeGen/SelectionDAG.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/CodeGen/SelectionDAG.h?rev=46585&r1=46584&r2=46585&view=diff
>
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- llvm/trunk/include/llvm/CodeGen/SelectionDAG.h (original)
> +++ llvm/trunk/include/llvm/CodeGen/SelectionDAG.h Wed Jan 30
> 18:25:39 2008
> @@ -381,8 +381,12 @@
> SDOperand getIndexedStore(SDOperand OrigStoe, SDOperand Base,
> SDOperand Offset, ISD::MemIndexedMode AM);
>
> + // getSrcValue - Construct a node to track a Value* through the
> backend.
> + SDOperand getSrcValue(const Value *v);
> +
> + // getMemOperand - Construct a node to track a memory reference
> + // through the backend.
> + SDOperand getMemOperand(const MemOperand &MO);
What is the difference between a SrcValueSDNode and a MemOperandSDNode
now? Is the former a special case of the later?
> +/// MemOperandSDNode - An SDNode that holds a MemOperand. This is
> +/// used to represent a reference to memory after ISD::LOAD
> +/// and ISD::STORE have been lowered.
> +///
> +class MemOperandSDNode : public SDNode {
> + virtual void ANCHOR(); // Out-of-line virtual method to give
> class a home.
> +protected:
> + friend class SelectionDAG;
> + /// Create a MemOperand node
> + explicit MemOperandSDNode(MemOperand mo)
This should probably take 'mo' by const reference to avoid a copy.
> @@ -1546,6 +1573,10 @@
> /// isUnindexed - Return true if this is NOT a pre/post inc/dec
> load/store.
> bool isUnindexed() const { return AddrMode == ISD::UNINDEXED; }
>
> + /// getMemOperand - Return a MemOperand object describing the
> memory
> + /// reference performed by this load or store.
> + MemOperand getMemOperand() const;
Would it make sense to merge all the fields in LSBaseSDNode into a
MemOperand ivar?
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- llvm/trunk/lib/CodeGen/PseudoSourceValue.cpp (original)
> +++ llvm/trunk/lib/CodeGen/PseudoSourceValue.cpp Wed Feb 6 16:27:42
> 2008
> @@ -0,0 +1,41 @@
...
> +namespace llvm {
> + static ManagedStatic<PseudoSourceValue[5]> PSVs;
> +
> + const PseudoSourceValue &PseudoSourceValue::getFixedStack()
> { return (*PSVs)[0]; }
> + const PseudoSourceValue &PseudoSourceValue::getStack() { return
> (*PSVs)[1]; }
> + const PseudoSourceValue &PseudoSourceValue::getGOT() { return
> (*PSVs)[2]; }
> + const PseudoSourceValue &PseudoSourceValue::getConstantPool()
> { return (*PSVs)[3]; }
> + const PseudoSourceValue &PseudoSourceValue::getJumpTable()
> { return (*PSVs)[4]; }
> +
80 col violations, but otherwise looks nice.
> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp?rev=46827&r1=46826&r2=46827&view=diff
>
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp (original)
> +++ llvm/trunk/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp Wed Feb 6
> 16:27:42 2008
> @@ -1316,8 +1320,15 @@
> MVT::ValueType IdxVT = Tmp3.getValueType();
> MVT::ValueType PtrVT = TLI.getPointerTy();
> SDOperand StackPtr = DAG.CreateStackTemporary(VT);
> +
> + FrameIndexSDNode *StackPtrFI =
> dyn_cast<FrameIndexSDNode>(StackPtr.Val);
> + assert(StackPtrFI);
Instead of dyn_cast + assert, please just use cast<>
> + int SPFI = StackPtrFI->getIndex();
> +
> // Store the vector.
> + SDOperand Ch = DAG.getStore(DAG.getEntryNode(), Tmp1, StackPtr,
> +
> &PseudoSourceValue::getFixedStack(),
> + SPFI);
This is a little bit strange to me: isn't this setting the "offset" of
the srcvalue to the frameindex? If this is expected, then please
document this really clearly in MemOperand and friends. This is
somewhat surprising to me (but not unreasonable), and any clients of
this information will be required to handle this specially.
> @@ -3240,16 +3255,14 @@
> }
> break;
> case TargetLowering::Expand: {
> - SrcValueSDNode *SV = cast<SrcValueSDNode>(Node->getOperand(2));
> - SDOperand VAList = DAG.getLoad(TLI.getPointerTy(), Tmp1, Tmp2,
> - SV->getValue(), SV-
> >getOffset());
> + const Value *V = cast<SrcValueSDNode>(Node->getOperand(2))-
> >getValue();
> + SDOperand VAList = DAG.getLoad(TLI.getPointerTy(), Tmp1,
> Tmp2, V, 0);
I think this is setting the offset incorrectly. Isn't SV->getOffset()
correct here instead of 0?
>
> // Increment the pointer, VAList, to the next vaarg
> Tmp3 = DAG.getNode(ISD::ADD, TLI.getPointerTy(), VAList,
> DAG.getConstant(MVT::getSizeInBits(VT)/8,
> TLI.getPointerTy()));
> // Store the incremented VAList to the legalized pointer
> - Tmp3 = DAG.getStore(VAList.getValue(1), Tmp3, Tmp2, SV-
> >getValue(),
> - SV->getOffset());
> + Tmp3 = DAG.getStore(VAList.getValue(1), Tmp3, Tmp2, V, 0);
Likewise, I think this should be "offset + MVT::getSizeInBits(VT)/8".
> @@ -3285,12 +3298,10 @@
> case TargetLowering::Expand:
> // This defaults to loading a pointer from the input and
> storing it to the
> // output, returning the chain.
> - SrcValueSDNode *SVD = cast<SrcValueSDNode>(Node-
> >getOperand(3));
> - SrcValueSDNode *SVS = cast<SrcValueSDNode>(Node-
> >getOperand(4));
> - Tmp4 = DAG.getLoad(TLI.getPointerTy(), Tmp1, Tmp3, SVD-
> >getValue(),
> - SVD->getOffset());
> - Result = DAG.getStore(Tmp4.getValue(1), Tmp4, Tmp2, SVS-
> >getValue(),
> - SVS->getOffset());
> + const Value *VD = cast<SrcValueSDNode>(Node->getOperand(3))-
> >getValue();
> + const Value *VS = cast<SrcValueSDNode>(Node->getOperand(4))-
> >getValue();
> + Tmp4 = DAG.getLoad(TLI.getPointerTy(), Tmp1, Tmp3, VD, 0);
> + Result = DAG.getStore(Tmp4.getValue(1), Tmp4, Tmp2, VS, 0);
This also seems to be dropping offset info, though maybe I'm missing
something?
> @@ -4285,16 +4296,14 @@
> Tmp3 = DAG.getVAArg(VT, Tmp1, Tmp2, Node->getOperand(2));
> Result = TLI.CustomPromoteOperation(Tmp3, DAG);
> } else {
> - SrcValueSDNode *SV = cast<SrcValueSDNode>(Node->getOperand(2));
> - SDOperand VAList = DAG.getLoad(TLI.getPointerTy(), Tmp1, Tmp2,
> - SV->getValue(), SV-
> >getOffset());
> + const Value *V = cast<SrcValueSDNode>(Node->getOperand(2))-
> >getValue();
> + SDOperand VAList = DAG.getLoad(TLI.getPointerTy(), Tmp1,
> Tmp2, V, 0);
Here too.
> // Increment the pointer, VAList, to the next vaarg
> Tmp3 = DAG.getNode(ISD::ADD, TLI.getPointerTy(), VAList,
> DAG.getConstant(MVT::getSizeInBits(VT)/8,
> TLI.getPointerTy()));
> // Store the incremented VAList to the legalized pointer
> - Tmp3 = DAG.getStore(VAList.getValue(1), Tmp3, Tmp2, SV-
> >getValue(),
> - SV->getOffset());
> + Tmp3 = DAG.getStore(VAList.getValue(1), Tmp3, Tmp2, V, 0);
Here too. I assume I'm missing something, so I'll stop commenting on
these.
> @@ -4750,6 +4759,10 @@
> // Create the stack frame object.
> SDOperand FIPtr = DAG.CreateStackTemporary(SlotVT);
>
> + FrameIndexSDNode *StackPtrFI = dyn_cast<FrameIndexSDNode>(FIPtr);
> + assert(StackPtrFI);
should be cast<>
> @@ -4776,9 +4793,15 @@
> // Create a vector sized/aligned stack slot, store the value to
> element #0,
> // then load the whole vector back out.
> SDOperand StackPtr = DAG.CreateStackTemporary(Node-
> >getValueType(0));
> +
> + FrameIndexSDNode *StackPtrFI =
> dyn_cast<FrameIndexSDNode>(StackPtr);
> + assert(StackPtrFI);
cast<>
> @@ -6743,10 +6773,16 @@
> // Lower to a store/load so that it can be split.
> // FIXME: this could be improved probably.
> SDOperand Ptr = DAG.CreateStackTemporary(InOp.getValueType());
> + FrameIndexSDNode *FI = dyn_cast<FrameIndexSDNode>(Ptr.Val);
> + assert(FI && "Expecting CreateStackTemporary to return a
> frame index.\n");
cast<>
I'll continue the review in a separate email,
-Chris
More information about the llvm-commits
mailing list