<div dir="ltr">Ping. Any comments?</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Mar 24, 2014 at 3:29 PM, Rui Ueyama <span dir="ltr"><<a href="mailto:ruiu@google.com" target="_blank">ruiu@google.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Bigcheese, shankarke, kledzik, atanasyan,<br>
<br>
Currently we use both layout-after and layout-before edges to specify atom<br>
orders in the resulting executable. We have a complex piece of code in<br>
LayoutPass.cpp to deal with both types of layout specifiers.<br>
<br>
(In the following description, I denote "Atom A having a layout-after edge<br>
to B" as "A -> B", and A's layout-before to B as "A => B".)<br>
<br>
However, that complexity is not really needed for this reason: If there<br>
are atoms such that A => B, B -> A is always satisifed, so using only layout-<br>
after relationships will yield the same result as the current code.<br>
<br>
Actually we have a piece of complex code that verifies that, for each A -> B,<br>
B => [ X => Y => ... => Z => ] A is satsified, where X, Y, ... Z are all<br>
zero-size atoms. We can get rid of the code from our codebase because layout-<br>
before is basically redundant.<br>
<br>
I think we can simplify the code for layout-after even more than this, but<br>
I want to just remove this pass for now for simplicity.<br>
<br>
Layout-before edges are still there for dead-stripping, so this change won't<br>
break it.<br>
<br>
<a href="http://llvm-reviews.chandlerc.com/D3164" target="_blank">http://llvm-reviews.chandlerc.com/D3164</a><br>
<br>
Files:<br>
include/lld/Passes/LayoutPass.h<br>
lib/Passes/LayoutPass.cpp<br>
test/core/layout-error-test.objtxt<br>
test/core/layoutbefore-test.objtxt<br>
<br>
Index: include/lld/Passes/LayoutPass.h<br>
===================================================================<br>
--- include/lld/Passes/LayoutPass.h<br>
+++ include/lld/Passes/LayoutPass.h<br>
@@ -55,10 +55,6 @@<br>
// reference type<br>
void buildInGroupTable(MutableFile::DefinedAtomRange &range);<br>
<br>
- // Build the PrecededBy Table as specified by the kindLayoutBefore<br>
- // reference type<br>
- void buildPrecededByTable(MutableFile::DefinedAtomRange &range);<br>
-<br>
// Build a map of Atoms to ordinals for sorting the atoms<br>
void buildOrdinalOverrideMap(MutableFile::DefinedAtomRange &range);<br>
<br>
Index: lib/Passes/LayoutPass.cpp<br>
===================================================================<br>
--- lib/Passes/LayoutPass.cpp<br>
+++ lib/Passes/LayoutPass.cpp<br>
@@ -159,8 +159,7 @@<br>
<br>
/// The function compares atoms by sorting atoms in the following order<br>
/// a) Sorts atoms by Section position preference<br>
-/// b) Sorts atoms by their ordinal overrides<br>
-/// (layout-after/layout-before/ingroup)<br>
+/// b) Sorts atoms by their ordinal overrides (layout-after/ingroup)<br>
/// c) Sorts atoms by their permissions<br>
/// d) Sorts atoms by their content<br>
/// e) Sorts atoms on how they appear using File Ordinality<br>
@@ -448,75 +447,6 @@<br>
}<br>
}<br>
<br>
-/// This pass builds the followon tables using Preceded By relationships<br>
-/// The algorithm follows a very simple approach<br>
-/// a) If the targetAtom is not part of any root and the current atom is not<br>
-/// part of any root, create a chain with the current atom as root and<br>
-/// the targetAtom as following the current atom<br>
-/// b) Chain the targetAtom to the current Atom if the targetAtom is not part<br>
-/// of any chain and the currentAtom has no followOn's<br>
-/// c) If the targetAtom is part of a different tree and the root of the<br>
-/// targetAtom is itself, and if the current atom is not part of any root<br>
-/// chain all the atoms together<br>
-/// d) If the current atom has no followon and the root of the targetAtom is<br>
-/// not equal to the root of the current atom(the targetAtom is not in the<br>
-/// same chain), chain all the atoms that are lead by the targetAtom into<br>
-/// the current chain<br>
-void LayoutPass::buildPrecededByTable(MutableFile::DefinedAtomRange &range) {<br>
- ScopedTask task(getDefaultDomain(), "LayoutPass::buildPrecededByTable");<br>
- // This table would convert precededby references to follow on<br>
- // references so that we have only one table<br>
- for (const DefinedAtom *ai : range) {<br>
- for (const Reference *r : *ai) {<br>
- if (r->kindNamespace() != lld::Reference::KindNamespace::all ||<br>
- r->kindValue() != lld::Reference::kindLayoutBefore)<br>
- continue;<br>
- const DefinedAtom *targetAtom = dyn_cast<DefinedAtom>(r->target());<br>
- // Is the targetAtom not chained<br>
- if (_followOnRoots.count(targetAtom) == 0) {<br>
- // Is the current atom not part of any root?<br>
- if (_followOnRoots.count(ai) == 0) {<br>
- _followOnRoots[ai] = ai;<br>
- _followOnNexts[ai] = targetAtom;<br>
- _followOnRoots[targetAtom] = _followOnRoots[ai];<br>
- continue;<br>
- }<br>
- if (_followOnNexts.count(ai) == 0) {<br>
- // Chain the targetAtom to the current Atom<br>
- // if the currentAtom has no followon references<br>
- _followOnNexts[ai] = targetAtom;<br>
- _followOnRoots[targetAtom] = _followOnRoots[ai];<br>
- }<br>
- continue;<br>
- }<br>
- if (_followOnRoots.find(targetAtom)->second != targetAtom)<br>
- continue;<br>
- // Is the targetAtom in chain with the targetAtom as the root?<br>
- bool changeRoots = false;<br>
- if (_followOnRoots.count(ai) == 0) {<br>
- _followOnRoots[ai] = ai;<br>
- _followOnNexts[ai] = targetAtom;<br>
- _followOnRoots[targetAtom] = _followOnRoots[ai];<br>
- changeRoots = true;<br>
- } else if (_followOnNexts.count(ai) == 0) {<br>
- // Chain the targetAtom to the current Atom<br>
- // if the currentAtom has no followon references<br>
- if (_followOnRoots[ai] != _followOnRoots[targetAtom]) {<br>
- _followOnNexts[ai] = targetAtom;<br>
- _followOnRoots[targetAtom] = _followOnRoots[ai];<br>
- changeRoots = true;<br>
- }<br>
- }<br>
- // Change the roots of the targetAtom and its chain to<br>
- // the current atoms root<br>
- if (changeRoots) {<br>
- setChainRoot(_followOnRoots[targetAtom], _followOnRoots[ai]);<br>
- }<br>
- }<br>
- }<br>
-}<br>
-<br>
-<br>
/// Build an ordinal override map by traversing the followon chain, and<br>
/// assigning ordinals to each atom, if the atoms have their ordinals<br>
/// already assigned skip the atom and move to the next. This is the<br>
@@ -572,9 +502,6 @@<br>
// Build Ingroup reference table<br>
buildInGroupTable(atomRange);<br>
<br>
- // Build preceded by tables<br>
- buildPrecededByTable(atomRange);<br>
-<br>
// Check the structure of followon graph if running in debug mode.<br>
DEBUG(checkFollowonChain(atomRange));<br>
<br>
Index: test/core/layout-error-test.objtxt<br>
===================================================================<br>
--- test/core/layout-error-test.objtxt<br>
+++ /dev/null<br>
@@ -1,22 +0,0 @@<br>
-# REQUIRES: asserts<br>
-# RUN: not lld -core --add-pass layout -mllvm -debug-only=LayoutPass \<br>
-# RUN: %s 2> %t.err<br>
-# RUN: FileCheck %s -check-prefix=CHECK < %t.err<br>
-<br>
----<br>
-defined-atoms:<br>
- - name: fn<br>
- scope: global<br>
- references:<br>
- - kind: layout-before<br>
- offset: 0<br>
- target: fn<br>
- - kind: in-group<br>
- offset: 0<br>
- target: fn<br>
-...<br>
-<br>
-# CHECK: There's a cycle in a follow-on chain!<br>
-# CHECK: fn<br>
-# CHECK: layout-before: fn<br>
-# CHECK: in-group: fn<br>
Index: test/core/layoutbefore-test.objtxt<br>
===================================================================<br>
--- test/core/layoutbefore-test.objtxt<br>
+++ /dev/null<br>
@@ -1,25 +0,0 @@<br>
-# RUN: lld -core --add-pass layout %s | FileCheck %s -check-prefix=CHKORDER<br>
-<br>
----<br>
-defined-atoms:<br>
- - name: fn<br>
- scope: global<br>
-<br>
- - name: fn1<br>
- scope: global<br>
- references:<br>
- - kind: layout-before<br>
- offset: 0<br>
- target: fn<br>
- - name: fn2<br>
- scope: global<br>
- references:<br>
- - kind: layout-before<br>
- offset: 0<br>
- target: fn1<br>
-...<br>
-<br>
-<br>
-# CHKORDER: - name: fn2<br>
-# CHKORDER: - name: fn1<br>
-# CHKORDER: - name: fn<br>
</blockquote></div><br></div>