On Apr 20, 2007, at 12:12 AM, Chris Lattner wrote:
On Apr 19, 2007, at 2:12 PM, Christopher Lamb wrote:
This patch is a bytecode change. It takes the volatile load/store
pseudo instructions and turns them into attributed load/store
instructions. Current attributes are either volatile and/or an
alignment.
The change is assembly format backwards compatible.
The change should require no changes in the front end, but the
front end does not currently use the align attribute.
Overall this looks really great. Please get Reid to review the bc
format changes when he gets back (likely after the patch goes in).
Some requests:
This adds an extra word to LoadInst and StoreInst. Since
alignments must be a power of two, I suggest representing them as a
shift amount (e.g, an alignment of 16 is stored as 4, 1<<4 == 16).
Given this, we don't need a large number of bits to support very
large shift amounts. Given this, you can store it in the
"SubClassData" member of Value (which is 16 bits wide), to avoid
using the word. Load and StoreInst currently store a bit in that
word. The only trick here is that we want power-of-two-plus one,
so that "0" represents an alignment of zero, not an alignment of 1.
Yeah, I thought about that. It's not a bit problem, I'll encode the
alignment the same way it's encoded in the BC which has this same issue.
Please don't #include MathExtras.h in Instructions.h
No prob. I'll have to move the accessor methods to the .cpp file and
include MathExtras there to use the Log2_32 function, though.
+ LoadInst(Value *Ptr, const std::string &Name, bool isVolatile =
false,
+ Instruction *InsertBefore = 0, unsigned Align = 0);
The instruction* to insert before should be the last member. In
the case of alignment, I don't think we need a helper constructor
for this (in fact, we probably don't need one for volatile
either). Any clients that want to create a load instruction with a
specific alignment can create it, then use setAlignment() :)
These helper constructors were used quite prolifically in the
lowering passes, like the hunk below. Do you prefer them to be
changed to set the attribute directly?
SDOperand SelectionDAGLowering::getLoadFrom(const Type *Ty,
SDOperand Ptr,
const Value *SV,
SDOperand Root,
bool isVolatile,
unsigned Alignment) {
...
} else {
- L = DAG.getLoad(TLI.getValueType(Ty), Root, Ptr, SV, 0,
isVolatile);
+ L = DAG.getLoad(TLI.getValueType(Ty), Root, Ptr, SV, 0,
isVolatile, Alignment);
}
Please ensure the code stays within 80 columns.
@@ -2526,11 +2548,10 @@ SDOperand DAGCombiner::visitTRUNCATE(SDN
// if the source and dest are the same type, we can drop
both the extend
// and the truncate
return N0.getOperand(0);
}
- // fold (truncate (load x)) -> (smaller load x)
// fold (truncate (srl (load x), c)) -> (smaller load (x+c/
evtbits))
return ReduceLoadWidth(N);
}
You don't like this line? :)
@@ -742,10 +738,34 @@ void BytecodeWriter::outputInstruction(c
...
+ if (LI->getAlignment() || LI->isVolatile()) {
+ NumOperands = 2;
+ Slots[1] = ((Log2_32(LI->getAlignment())+1)<<1) + (LI-
>isVolatile() ? 1 : 0);
Long line
...
+ // We need to encode the attributes of the store instruction
as the third
+ // operand. Its not really a slot, but we don't want to
break the
+ // instruction format for these instructions.
+ if (SI->getAlignment() || SI->isVolatile()) {
+ NumOperands = 3;
+ Slots[2] = ((Log2_32(SI->getAlignment())+1)<<1) + (SI-
>isVolatile() ? 1 : 0);
Long line.
Please also update LangRef.html and the Bytecodeformat.html
document as well.
The patch looks great. Please update it the include the above,
then commit it when you're ready. Thanks!
No problem.
--
Christopher Lamb
_______________________________________________
llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
