[llvm-commits] Deterministic finite automaton based packetizer for VLIW architectures

[Anshuman Dasgupta]

Hi Jakob,

Thanks for your feedback.

 > I was missing documentation in your patch. What does the DFA do? How 
is it generated?
 > How can I use it? I think this deserves a full section 
in docs/CodeGenerator.html
 >
 > I would like to review the functionality as well, but that will be a 
lot easier if you
 > submit a new patch with documentation first.

Sure, I've attached documentation on the functionality of the DFA. The 
document presents an overview of how the DFA is constructed and what it 
does. I will work on a patch to CodeGenerator.html that will describe 
the DFA generator but this document should let you review the code's 
functionality.

 > Please make sure your braces and indentation matches the rest of 
LLVM. You also have a couple of 80-col violations.

Will do.


 > +#include <iostream>
 > Please read http://llvm.org/docs/CodingStandards.html

Yes, sorry about that. I'll fix that.

 > You are using std::set and std::map a lot. These containers are quite 
expensive, and should
 > only be used if you actually need features that the llvm/ADT classes 
don't offer. They do get
 > used more in TableGen where performance is less critical, but still.

Okay, I'll work on changing these containers over to the llvm equivalents.


 > Is this set used anywhere but here?:
 > +  // Add the new transition
 > +  transitions.insert(T);
 > If not, you can optimize that ;-)

hah! Yeah, I had added that for sanity checking. I'll remove it :)


 > +namespace llvm {
 > +  typedef std::pair<unsigned, unsigned> UnsignPair;
 > Please don't put trivial typedefs like that in the llvm namespace. 
Either move the type
 > into your class, or just spell it out in the header.
 >
 > +++ b/utils/TableGen/DFAPacketizerEmitter.h
 > +typedef unsigned Input;
 > Don't put this in the global namespace, or even in the llvm namespace.

Okay, will change the code.

 > +class State {
 > +struct Transition {
 > +class DFA {
 >
 > Even for TableGen, these names are too generic to go in the llvm 
namespace.

Okay, I'll think of more specific names probably along the lines of 
prepending VLIWPacketizer to these names.

Thanks
-Anshu






On 11/28/2011 1:32 PM, Jakob Stoklund Olesen wrote:
>
> On Nov 18, 2011, at 8:51 PM, Anshuman Dasgupta wrote:
>
>> I'm attaching a patch that adds support for a deterministic finite 
>> automaton (DFA) based packetizer for VLIW architectures. 
>> Specifically, it automatically generates a DFA from a VLIW target's 
>> Schedule.td file. In a VLIW machine, an instruction can typically be 
>> dispatched to one or many function units. The DFA determines whether 
>> there exists a legal mapping of instructions to functional unit 
>> assignments in a packet. This DFA can then be queried by a backend 
>> packetization pass to determine which instructions can be grouped 
>> into a VLIW packet.
>>
>> This patch contains the machine-independent code that adds a 
>> component to TableGen. The component autogenerates the DFA and 
>> implements the API that can be used to query the DFA during 
>> packetization. This can be used by any VLIW target to packetize its 
>> instructions. After the Hexagon backend is committed, I will post 
>> another Hexagon-specific patch that uses this mechanism to construct 
>> packets in the Hexagon backend. I ran the llvm test suite and this 
>> patch did not cause any regressions.
>>
>> I would appreciate reviews and comments.
>
> Hi Anshu,
>
> Thanks for making this target-independent. It looks interesting.
>
> I was missing documentation in your patch. What does the DFA do? How 
> is it generated? How can I use it? I think this deserves a full 
> section in docs/CodeGenerator.html
>
> +struct ltState
> +{
>
> +class DFA {
> + public:
>
> Please make sure your braces and indentation matches the rest of LLVM. 
> You also have a couple of 80-col violations.
>
> You are using std::set and std::map a lot. These containers are quite 
> expensive, and should only be used if you actually need features that 
> the llvm/ADT classes don't offer. They do get used more in TableGen 
> where performance is less critical, but still.
>
>
> +#include <iostream>
>
> Please read http://llvm.org/docs/CodingStandards.html
>
>
> +  // Set of transitions
> +  std::set<Transition*> transitions;
>
> Is this set used anywhere but here?:
>
> +  // Add the new transition
> +  transitions.insert(T);
>
> If not, you can optimize that ;-)
>
>
> +namespace llvm {
> +  typedef std::pair<unsigned, unsigned> UnsignPair;
>
> Please don't put trivial typedefs like that in the llvm namespace. 
> Either move the type into your class, or just spell it out in the header.
>
>
> +++ b/utils/TableGen/DFAPacketizerEmitter.h
> +typedef unsigned Input;
>
> Don't put this in the global namespace, or even in the llvm namespace.
>
>
> +class State {
> +struct Transition {
> +class DFA {
>
> Even for TableGen, these names are too generic to go in the llvm 
> namespace.
>
> I would like to review the functionality as well, but that will be a 
> lot easier if you submit a new patch with documentation first.
>
> Thanks,
> /jakob
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20111128/f2dcb957/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: DFA Packetizer Design.pdf
Type: application/pdf
Size: 349908 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20111128/f2dcb957/attachment.pdf>