[llvm-commits] PR400 align attribute

[Chris Lattner]

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.


Please don't #include MathExtras.h in Instructions.h


+  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() :)



  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!

-Chris
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20070419/fe338999/attachment.html>