<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
On 4/23/2012 3:26 PM, Andrew Trick wrote:
<blockquote
cite="mid:14F368AF-A60C-4EDD-A6F0-C61E9DEA5891@apple.com"
type="cite"><br>
<div>
<div>On Apr 23, 2012, at 11:55 AM, Sirish Pande <<a
moz-do-not-send="true" href="mailto:spande@codeaurora.org">spande@codeaurora.org</a>>
wrote:</div>
<br class="Apple-interchange-newline">
<blockquote type="cite">
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
<div bgcolor="#FFFFFF" text="#000000"> On 4/23/2012 1:29 PM,
Chandler Carruth wrote:
<blockquote
cite="mid:CAGCO0KiD8Y8Pq0VHwGhWBzJdHR=p=8xpj91X_bT4qffTLtCF0Q@mail.gmail.com"
type="cite">
<div class="gmail_extra">
<div class="gmail_quote">On Mon, Apr 23, 2012 at 11:09
AM, Sirish Pande <span dir="ltr"><<a
moz-do-not-send="true"
href="mailto:spande@codeaurora.org"
target="_blank">spande@codeaurora.org</a>></span>
wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0
.8ex;border-left:1px #ccc solid;padding-left:1ex">
<div bgcolor="#FFFFFF" text="#000000"> There were
only two reviews:<br>
<br>
1. Eric C asked for test cases, and that test
cases be associated with the patch.<br>
2. Tom Stellard had requested to split the vliw
packetizer patch.<br>
<br>
Both have been done.<br>
</div>
</blockquote>
<div><br>
</div>
<div>But the review isn't finished yet. You need to
wait for the reviewers to actually sign off on the
change before submitting. The two comments you got
were what people needed changed based on their first
look, that doesn't mean the code is ready to be
submitted yet.</div>
<div><br>
</div>
<div>This patch is particularly problematic because it
is not just changing the Hexagon, it's changing a
core part of the codegen layer. Those changes need
proper review before being committed, and that
hasn't happened yet.</div>
<div><br>
</div>
<div>I've reverted this patch until the review
finishes because the patches requiring it had to be
reverted anyways.</div>
<div><br>
</div>
<div>I reverted the hexagon patches because your own
regression tests failed for all of the patches in
the series.</div>
<div><br>
</div>
<div>This gets the tree back into both a reviewed and
test-passing state. Let's keep it that way. =]</div>
</div>
</div>
</blockquote>
<br>
Fair enough. I will wait for the reviewers for sign off. <br>
</div>
</blockquote>
</div>
<br>
<div>
<div>This target-independent code looks ok to commit. (I haven't
looked at your target-specific patches yet.)</div>
<div><br>
</div>
<div>As usual, under the following conditions</div>
<div>- no build warnings</div>
<div>- try to avoid significant increase in build times</div>
<div>- make check-all passes when configured for *all* targets</div>
<div> (this should apply to everyone committing target
sensitive code, not just you!)</div>
<div>- Address buildbot failures that occured last time</div>
<div><br>
</div>
<div>Chandler mentioned that some bots showed failures, so
please look into those and don't commit until you think you've
fixed them.</div>
<div><br>
</div>
<div>A couple comments...</div>
<div><br>
</div>
<div>+ VLIWScheduler->exitRegion();</div>
<div><br>
</div>
<div>I'm not sure why you would do this *before* packetizing.
It's ok if it works, just a little misleading. You should
probably add a call to VLIWScheduler->finishBlock()
somewhere.</div>
</div>
</blockquote>
<br>
Thanks for pointing out about the finishBlock(). I will add
finishBlock() at the end of PacketizeMI function. <br>
<br>
You probably know that we are getting the dependence information
from the scheduler. Once we have the dependence info, along with the
DFA resource manager, we can packetize the way our architecture
prefers. We are not using the scheduler to *schedule* instructions
during packetization. Hence, we have enterRegion() and exitRegion()
around schedule. Esssentially, this logic is borrowed from around
line 200 of MachineScheduler.cpp<br>
<blockquote
cite="mid:14F368AF-A60C-4EDD-A6F0-C61E9DEA5891@apple.com"
type="cite">
<div>
<div><br>
</div>
<div>+ // Generate MI -> SU map.</div>
<div>+ //std::map <MachineInstr*, SUnit*> MIToSUnit;</div>
<div>+ MIToSUnit.clear();</div>
<div>+ for (unsigned i = 0, e =
VLIWScheduler->SUnits.size(); i != e; ++i) {</div>
<div>+ SUnit *SU = &VLIWScheduler->SUnits[i];</div>
<div>+ MIToSUnit[SU->getInstr()] = SU;</div>
<div>+ }</div>
<div><br>
</div>
<div>I added ScheduleDAGInstrs::MISUnitMap so you wouldn't need
your own. I haven't tested it with a PostRA scheduler, but
would like to know if it doesn't work.</div>
</div>
</blockquote>
<br>
Have you checked this functionality in? I don't see it.<br>
<blockquote
cite="mid:14F368AF-A60C-4EDD-A6F0-C61E9DEA5891@apple.com"
type="cite">
<div>
<div><br>
</div>
<div>Thanks!</div>
<div><br>
</div>
<div>-Andy</div>
</div>
<div><br>
</div>
</blockquote>
<br>
<br>
<pre class="moz-signature" cols="72">--
Qualcomm Innovation Center, Inc is a member of Code Aurora Forum</pre>
</body>
</html>