<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<tt>Hi Jakob,<br>
<br>
Thanks for your feedback.<br>
<br>
> I was missing documentation in your patch. What does the DFA
do? How is it generated?<br>
> How can I use it? I think this deserves a full section
in docs/CodeGenerator.html<br>
><br>
> I would like to review the functionality as well, but that
will be a lot easier if you<br>
> submit a new patch with documentation first.<br>
<br>
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.<br>
<br>
> </tt><tt>Please make sure your braces and indentation
matches the rest of LLVM. You also have a couple of 80-col
violations.<br>
<br>
Will do.<br>
</tt><tt><br>
<br>
</tt>
<div><tt>> +#include <iostream></tt></div>
<div><tt>> Please read <a moz-do-not-send="true"
href="http://llvm.org/docs/CodingStandards.html">http://llvm.org/docs/CodingStandards.html</a></tt></div>
<div><tt><br>
Yes, sorry about that. I'll fix that.<br>
</tt></div>
<tt><br>
</tt>
<div><tt>> You are using std::set and std::map a lot. These
containers are quite expensive, and should<br>
> only be used if you actually need features that the
llvm/ADT classes don't offer. They do get<br>
> used more in TableGen where performance is less critical,
but still.</tt></div>
<div><tt><br>
</tt></div>
<tt>Okay, I'll work on changing these containers over to the llvm
equivalents.<br>
<br>
</tt><tt><br>
</tt>
<div><tt>> Is this set used anywhere but here?:</tt></div>
<div><tt>> + // Add the new transition</tt></div>
<div>
<div><tt>> + transitions.insert(T);</tt></div>
</div>
<div><tt>> If not, you can optimize that ;-)</tt></div>
<tt><br>
hah! Yeah, I had added that for sanity checking. I'll remove it :)<br>
<br>
<br>
</tt>
<div>
<div><tt>> +namespace llvm {</tt></div>
<div><tt>> + typedef std::pair<unsigned, unsigned>
UnsignPair;</tt></div>
</div>
<div><tt>> Please don't put trivial typedefs like that in the
llvm namespace. Either move the type <br>
> into your class, or just spell it out in the header.<br>
><br>
</tt></div>
<div>
<div><tt>> +++ b/utils/TableGen/DFAPacketizerEmitter.h</tt></div>
<tt>
</tt>
<div><tt>> +typedef unsigned Input;</tt></div>
<tt>
</tt>
<div><tt>> Don't put this in the global namespace, or even in
the llvm namespace.</tt></div>
<tt>
<br>
</tt><tt>Okay, will change the code.<br>
</tt></div>
<tt><br>
</tt>
<div>
<div><tt>> +class State {</tt></div>
</div>
<div>
<div><tt>> +struct Transition {</tt></div>
</div>
<div>
<div><tt>> +class DFA {</tt></div>
</div>
<div><tt>><br>
</tt></div>
<div><tt>> Even for TableGen, these names are too generic to go
in the llvm namespace.</tt></div>
<div><tt><br>
Okay, I'll think of more specific names probably along the lines
of prepending VLIWPacketizer to these names.<br>
<br>
Thanks<br>
-Anshu<br>
<br>
<br>
<br>
</tt></div>
<tt><br>
<br>
<br>
On 11/28/2011 1:32 PM, Jakob Stoklund Olesen wrote:</tt>
<blockquote cite="mid:B80B85AF-547A-4A07-B9E1-F89C659CCA67@2pi.dk"
type="cite"><tt><br>
</tt>
<div>
<div><tt>On Nov 18, 2011, at 8:51 PM, Anshuman Dasgupta wrote:</tt></div>
<tt><br class="Apple-interchange-newline">
</tt>
<blockquote type="cite">
<meta http-equiv="content-type" content="text/html;
charset=ISO-8859-1">
<div bgcolor="#FFFFFF" text="#000000"><tt>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.<br>
<br>
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.<br>
<br>
I would appreciate reviews and comments.<br>
</tt></div>
</blockquote>
<div><tt><br>
</tt></div>
<div><tt>Hi Anshu,</tt></div>
<div><tt><br>
</tt></div>
<div><tt>Thanks for making this target-independent. It looks
interesting.</tt></div>
<div><tt><br>
</tt></div>
<div><tt>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</tt></div>
<div><tt><br>
</tt></div>
<div>
<div><tt>+struct ltState</tt></div>
<div><tt>+{</tt></div>
<div><tt><br>
</tt></div>
<div>
<div><tt>+class DFA {</tt></div>
<div><tt>+ public:</tt></div>
<div><tt><br>
</tt></div>
<div><tt>Please make sure your braces and indentation
matches the rest of LLVM. You also have a couple of
80-col violations.</tt></div>
<div><tt><br>
</tt></div>
<div><tt>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.</tt></div>
<div><tt><br>
</tt></div>
<div><tt><br>
</tt></div>
<div>
<div><tt>+#include <iostream></tt></div>
<div><tt><br>
</tt></div>
<div><tt>Please read <a moz-do-not-send="true"
href="http://llvm.org/docs/CodingStandards.html">http://llvm.org/docs/CodingStandards.html</a></tt></div>
<div><tt><br>
</tt></div>
<div><tt><br>
</tt></div>
<div>
<div><tt>+ // Set of transitions</tt></div>
<div><tt>+ std::set<Transition*> transitions;</tt></div>
<div><tt><br>
</tt></div>
</div>
<div><tt>Is this set used anywhere but here?:</tt></div>
<div><tt><br>
</tt></div>
<div>
<div><tt>+ // Add the new transition</tt></div>
<div><tt>+ transitions.insert(T);</tt></div>
<div><tt><br>
</tt></div>
</div>
<div><tt>If not, you can optimize that ;-)</tt></div>
<div><tt><br>
</tt></div>
<div><tt><br>
</tt></div>
<div>
<div><tt>+namespace llvm {</tt></div>
<div><tt>+ typedef std::pair<unsigned, unsigned>
UnsignPair;</tt></div>
<div><tt><br>
</tt></div>
</div>
<div><tt>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.</tt></div>
<div><tt><br>
</tt></div>
<div><tt><br>
</tt></div>
<div>
<div><tt>+++ b/utils/TableGen/DFAPacketizerEmitter.h</tt></div>
</div>
<div><tt>+typedef unsigned Input;</tt></div>
<div><tt><br>
</tt></div>
<div><tt>Don't put this in the global namespace, or even
in the llvm namespace.</tt></div>
<div><tt><br>
</tt></div>
<div><tt><br>
</tt></div>
<div>
<div><tt>+class State {</tt></div>
</div>
<div>
<div><tt>+struct Transition {</tt></div>
</div>
<div>
<div><tt>+class DFA {</tt></div>
</div>
<div><tt><br>
</tt></div>
<div><tt>Even for TableGen, these names are too generic to
go in the llvm namespace.</tt></div>
<div><tt><br>
</tt></div>
<div><tt>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.</tt></div>
<div><tt><br>
</tt></div>
<div><tt>Thanks,</tt></div>
<div><tt>/jakob</tt></div>
<div><tt><br>
</tt></div>
</div>
</div>
</div>
</div>
</blockquote>
<tt><br>
</tt>
</body>
</html>