[llvm-commits] PR400 align attribute

[Christopher Lamb]

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



-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20070420/28ce609d/attachment.html>