Re: [llvm-commits] [llvm] r46827 - memoperands #1
This, and the question of whether to make LSBaseNode store a MemOperand instead of separate fields, are related. Ok, right. What is your opinion on this? Is there any reason not to give MemOperand a VT and then give LSBaseNode a MemOperand? There's a little redundancy; the MemOperand has flags to distinguish between load and store, and LSBaseNode has its opcode which is either LOAD or STORE. But that's not a big problem. True, I don't think that an extra flag of redundancy is a big deal here. Code simplicity is worth it :) Good question. This sort of thing is currently rare enough that it is probably fine to just use a null Value*, and have everything treat it conservatively. Would this be acceptable for now? Yes, using a null Value* is done in many places right now that don't yet provide a proper SourceValue. Ok, -Chris ___ llvm-commits mailing list llvm-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
Re: [llvm-commits] [llvm] r46827 - memoperands #1
On Feb 12, 2008, at 1:42 PM, Chris Lattner wrote: On Feb 12, 2008, at 11:27 AM, Dan Gohman wrote: Hi Chris, Thanks for the careful review! I've responded to parts of it already, and I'll be responding to more soon. Thanks Dan! On Feb 10, 2008, at 11:56 AM, Chris Lattner wrote: 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. This, and the question of whether to make LSBaseNode store a MemOperand instead of separate fields, are related. Ok, right. What is your opinion on this? Is there any reason not to give MemOperand a VT and then give LSBaseNode a MemOperand? There's a little redundancy; the MemOperand has flags to distinguish between load and store, and LSBaseNode has its opcode which is either LOAD or STORE. But that's not a big problem. Also related is the question is what to do about the lowering of something like insert vector element where the element index isn't a constant and the target doesn't have an instruction to handle it. Legalize emits a store with a computed offset; what should the MemOperand for this look like? One way is to give it a larger size, to cover the known area over which the store might occur. This would mean it would use a different VT from the actual store, which could be confusing. Maybe it should have both a size and a VT. Good question. This sort of thing is currently rare enough that it is probably fine to just use a null Value*, and have everything treat it conservatively. Would this be acceptable for now? Yes, using a null Value* is done in many places right now that don't yet provide a proper SourceValue. 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 #1
Hi Chris, Thanks for the careful review! I've responded to parts of it already, and I'll be responding to more soon. On Feb 10, 2008, at 11:56 AM, Chris Lattner wrote: 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. This, and the question of whether to make LSBaseNode store a MemOperand instead of separate fields, are related. Also related is the question is what to do about the lowering of something like insert vector element where the element index isn't a constant and the target doesn't have an instruction to handle it. Legalize emits a store with a computed offset; what should the MemOperand for this look like? One way is to give it a larger size, to cover the known area over which the store might occur. This would mean it would use a different VT from the actual store, which could be confusing. Maybe it should have both a size and a VT. 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 #1
On Feb 12, 2008, at 11:27 AM, Dan Gohman wrote: Hi Chris, Thanks for the careful review! I've responded to parts of it already, and I'll be responding to more soon. Thanks Dan! On Feb 10, 2008, at 11:56 AM, Chris Lattner wrote: 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. This, and the question of whether to make LSBaseNode store a MemOperand instead of separate fields, are related. Ok, right. What is your opinion on this? Is there any reason not to give MemOperand a VT and then give LSBaseNode a MemOperand? Also related is the question is what to do about the lowering of something like insert vector element where the element index isn't a constant and the target doesn't have an instruction to handle it. Legalize emits a store with a computed offset; what should the MemOperand for this look like? One way is to give it a larger size, to cover the known area over which the store might occur. This would mean it would use a different VT from the actual store, which could be confusing. Maybe it should have both a size and a VT. Good question. This sort of thing is currently rare enough that it is probably fine to just use a null Value*, and have everything treat it conservatively. Would this be acceptable for now? -Chris ___ llvm-commits mailing list llvm-commits@cs.uiuc.edu http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
Re: [llvm-commits] [llvm] r46827 - memoperands #1
On Feb 6, 2008, at 2:27 PM, Dan Gohman wrote:
URL: http://llvm.org/viewvc/llvm-project?rev=46827view=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=46585r1=46584r2=46585view=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 ManagedStaticPseudoSourceValue[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=46827r1=46826r2=46827view=diff
=
=
=
=
=
=
=
=
==
---
