<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 4 June 2014 15:57, Stepan Dyatkovskiy <span dir="ltr"><<a href="mailto:stpworld@narod.ru" target="_blank">stpworld@narod.ru</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Hi Nick,<br>
<br>
What about rest of patches? I rescanned remained patches, updated them (according to trunk). I have also added few more comments in 0010 about how finally we introduce binary tree and N*log(N) time.<br>
All the patches are reattached to this post.<br></blockquote><div><br></div><div><br></div><div>0006 LGTM.</div><div><br></div><div>0007: Please wrap it in DEBUG(...). How bad would it be to hoist it out to a new function and then just wrap the call to it with DEBUG()? I think you just need to pass in Worklist?</div>
<div><br></div><div><div>+ if (Res1 != 0 && Res1 == Res4) {</div><div>+ Transitive = Res3 == Res1;</div><div>+ } else</div><div>+ // F1 > F3, F3 > F2 => F1 > F2</div>
<div>+ if (Res3 != 0 && Res3 == -Res4) {</div><div>+ Transitive = Res3 == Res1;</div><div>+ } else</div><div>+ // F2 > F3, F3 > F1 => F2 > F1</div>
<div>+ if (Res4 != 0 && -Res3 == Res4) {</div><div>+ Transitive = Res4 == -Res1;</div><div>+ }</div><div>+</div></div><div><br></div><div>The comments between the "else" and the "if" in "} else if {" make the code really hard to read. Please just put the comments in the code block after the {.</div>
<div><br></div><div>if (Res1 != 0 && Res1 == Res4) {</div><div> Transitive = Res3 == Res1;</div><div>} else if (Res3 != 0 && Res3 == -Res4) {</div><div> // F1 > F3, F3 > F2, F2 => F1 > F2</div>
<div> Transitive = Res3 == Res1;</div><div>} else if (Res4 != 0 && -Res3 == Res4) {</div><div> // F2 > F3, F3 > F1 => F2 > F1</div><div> Transitive = Res4 == -Res1;</div><div>}</div><div><br></div>
<div>
0008: LGTM</div><div><br></div><div>0009: LGTM</div><div><br></div><div>0010: </div><div><br></div><div>+// In practice it works with next way:<br></div><div><br></div><div>"In practice it works the following way:"</div>
<div><br></div><div>+// could be cover much more cases.<br></div><div><br></div><div>"could be cover" --> "could cover".</div><div><br></div><div>0011: LGTM</div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
Thanks!<br>
<br>
-Stepan.<br>
<br>
Stepan Dyatkovskiy wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Hi Nick,<br>
Thanks for reviews!<br>
<br>
I have committed 0003 - 0005 as 208953, 208973 and 208976.<br>
<br>
> + // TODO: Already checked in cmpOps<br>
Yup. I have fixed it with "Already checked in cmpOperation".<br>
The reason I put this TODO, is because we don't need this check,<br>
operation types were already checked in cmpOperation. But I didn't<br>
remove this code, because its not mine (it was in old code as well), and<br>
actually it is subject of additional cosmetic commit.<br>
<br>
Thanks!<br>
-Stepan<br>
<br>
Nick Lewycky wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
0003:<br>
<br>
+ // Mostly cloned from BitcodeWriter, but simplified a bit.<br>
<br>
Please remove that comment, it isn't adding anything for the reader.<br>
<br>
And submit.<br>
<br>
0004:<br>
<br>
I *really* like the comment block!! This is going to save developers so<br>
much time!<br>
<br>
No changes, please submit!<br>
<br>
0005:<br>
<br>
Perfect. Please submit.<br>
<br>
0006:<br>
<br>
+ // TODO: Already checked in cmpOps<br>
<br>
cmpOps?<br>
<br>
Nick<br>
<br>
On 15 May 2014 06:32, Stepan Dyatkovskiy <<a href="mailto:stpworld@narod.ru" target="_blank">stpworld@narod.ru</a><br>
<mailto:<a href="mailto:stpworld@narod.ru" target="_blank">stpworld@narod.ru</a>>> wrote:<br>
<br>
Hi Nick,<br>
What about rest of patches? I have fixed 0004 and 0005, others seems<br>
to be OK for you.. Can I commit them?<br>
<br>
Thanks!<br>
-Stepan<br>
<br>
Stepan Dyatkovskiy wrote:<br>
<br>
Hi Nick,<br>
Thanks you for reviews. I have corrected for of comments from<br>
FAQ to<br>
what seems to be regular in LLVM.<br>
Patches has been committed as r208173 and r208189.<br>
<br>
Thanks!<br>
-Stepan<br>
<br>
Nick Lewycky wrote:<br>
<br>
On 29 April 2014 04:36, Stepan Dyatkovskiy<br>
<<a href="mailto:stpworld@narod.ru" target="_blank">stpworld@narod.ru</a> <mailto:<a href="mailto:stpworld@narod.ru" target="_blank">stpworld@narod.ru</a>><br>
<mailto:<a href="mailto:stpworld@narod.ru" target="_blank">stpworld@narod.ru</a> <mailto:<a href="mailto:stpworld@narod.ru" target="_blank">stpworld@narod.ru</a>>>> wrote:<br>
<br>
Hi Nick.<br>
<br>
I have fixed and reattached all the patches from 0001<br>
till 0011.<br>
<br>
0001, 0002:<br>
I have added detailed order descriptions to the methods<br>
declaration.<br>
I have also fixed everything as you asked.<br>
<br>
<br>
0001:<br>
<br>
+ for (uint64_t i = 0; i < NumElementsL; ++i) {<br>
vs.<br>
+ for (unsigned i = 0, e = NumElementsL; i != e; ++i) {<br>
<br>
This difference is necessary to avoid a warning? I don't<br>
actually care<br>
which form you use here given that NumElementsL is already<br>
an integer<br>
there's no recalculation we're avoiding like getting the end<br>
iterator<br>
each time, I just want it to look consistent. If it's<br>
working around a<br>
warning that's more important than consistent looks.<br>
<br>
0001 looks good. Please commit it.<br>
<br>
0002:<br>
<br>
One note, you use a writing style of asking a question,<br>
followed by<br>
answering it. For instance, "Will we still get the same<br>
numbering? As<br>
follows from FunctionComparator::compare(), we do CFG walk,<br>
...". I<br>
don't think it's worth your time to rewrite it now, but we<br>
generally<br>
don't use that construction in llvm's comments, instead<br>
preferring to<br>
explain what the code does an including an argument that<br>
explains why<br>
the approach is correct. This amounts to the same thing,<br>
it's just a<br>
different style.<br>
<br>
+ /// their functions. If right value was meet<br>
first during<br>
scanning,<br>
<br>
"was meet" --> "was met"<br>
<br>
Please commit it.<br>
<br>
Nick<br>
<br>
<br>
0003, cmpStrings<br>
StringRef::compare performs lexicographical comparison,<br>
and it isn't<br>
the fastest one.<br>
In case, when strings has different length, it still<br>
compares their<br>
prefixes lexicographically.<br>
We need ordering among strings set, but we don't need<br>
to compare<br>
strings contents lexicographically all the time. So we<br>
can compare<br>
their sizes first, and do lexicographical comparison<br>
only when we<br>
really need it: only when strings are equal in size.<br>
<br>
I have added short version of these explanations into<br>
cmpStrings. So<br>
we could get really well documented methods based on<br>
our posts ;-)<br>
Those were good questions and definitely the answers<br>
should be in<br>
documentation somewhere.<br>
<br>
0004: I have added ordering description.<br>
0005: Fixed.<br>
0007: Fixed.<br>
0009:<br>
<br>
> Okay, I'm confused. We do all this work to produce a<br>
3-way<br>
comparison,<br>
> then we wrap it up in an operator< for a std::set<br>
that will only<br>
look at<br>
> less-than...<br>
Actually, on top level we don't need 3-way comparison.<br>
But we do<br>
need it to determine whether functions are equal on<br>
some stage, just<br>
to proceed to the next "less significant" stage.<br>
Consider two functions (where BB1 and BB11 are equal)<br>
<br>
"F1: {BB1, BB2}" and<br>
"F2: {BB11, BB3}".<br>
<br>
First we have to realize that BB1 and BB11 are equal.<br>
And then<br>
determine which one of BB2 and BB3 is less than another.<br>
So finally we got 3-way comparison: less, equal and<br>
greater (in fact<br>
just neither less nor equal).<br>
<br>
<br>
> When we start detecting equal functions, do we<br>
merge them<br>
immediately? If so, can't that cause functions already<br>
in the set to<br>
have their equality comparisons change<br>
Yes we merge them immediately. And no, it won't cause<br>
situation you<br>
have mentioned. Once we changed function X, we lookup<br>
for functions<br>
that uses X, remove them from FnTree (former FnSet) and<br>
put them<br>
into Deferred list. Then we fill up worklist with<br>
Deferred contents<br>
and repeat comparison routine for these functions.<br>
<br>
<br>
Thanks for reviews!<br>
<br>
-Stepan<br>
<br>
Stepan Dyatkovskiy wrote:<br>
<br>
ping<br>
Stepan Dyatkovskiy wrote:<br>
<br>
Hi Nick,<br>
Thank you for reviews! Here is my answers for<br>
first two<br>
patches.<br>
<br>
Though as additional, I think its important<br>
answer to this<br>
question<br>
first:<br>
> Okay, I'm confused. We do all this work to<br>
produce a<br>
3-way comparison,<br>
> then we wrap it up in an operator< for a<br>
std::set that<br>
will only<br>
look at<br>
> less-than...<br>
Actually, on top level we don't need 3-way<br>
comparison. But<br>
we do need it<br>
to determine whether functions are equal on<br>
some stage just<br>
to proceed<br>
to the next "less meaningful" stage.<br>
Consider two functions<br>
"F1: {BB1, BB2}" and<br>
"F2: {BB-equal-to-BB1, BB3}".<br>
First we have to realize that BB1 and<br>
BB-equal-to-BB1 are<br>
equal. And<br>
then determine which one of BB2 and BB3 is less.<br>
<br>
First patch.<br>
<br>
> + /// 1. Bitcastability.<br>
> + /// Check whether L's type could be<br>
losslessly<br>
bitcasted to R's<br>
type.<br>
> + /// On this stage method, in case when<br>
lossless<br>
bitcast is not<br>
possible<br>
> + /// method returns -1 or 1, thus also<br>
defining which<br>
type is<br>
greater in<br>
> + /// context of bitcastability.<br>
I have added detailed order description for<br>
this method. You<br>
also can<br>
find this description in<br>
cmpConstantsComment.txt.<br>
<br>
> Optional: "Return its pointers comparison<br>
result" -><br>
"Return the<br>
result<br>
> of comparing the types"?<br>
Oops! Fixed.<br>
<br>
> + if (TyL->getTypeID() ==<br>
Type::X86_MMXTyID)<br>
> + TyLWidth = 64;<br>
> + else if (const VectorType *VecTyL =<br>
dyn_cast<VectorType>(TyL))<br>
> + TyLWidth = VecTyL->getBitWidth();<br>
><br>
> Yikes! We never merge function A using a<br>
vector and<br>
function B<br>
using an<br>
> x86_mmx type together, right?<br>
Fixed!<br>
<br>
> "Nor of TyL and TyR" -> "Neither Tyl nor<br>
TyR".<br>
><br>
> Optional: "Return its pointers comparison<br>
result" -><br>
"Return the<br>
result<br>
> of comparing the types"?<br>
Fixed.<br>
<br>
> + case Value::UndefValueVal: return<br>
cmpType(L->getType(),<br>
R->getType());<br>
><br>
> You already have L->getType() and<br>
R->getType() in TyL<br>
and TyR.<br>
Fixed. Sorry.<br>
<br>
Second patch.<br>
<br>
> I'm curious whether we guarantee that<br>
> we get the same numbering when comparing<br>
two functions<br>
if I simply<br>
> reorder the BasicBlocks in one of the<br>
functions, will we<br>
still assign<br>
> the same numberings or not?<br>
...<br>
> Why not just start at the beginning and<br>
run to<br>
> the end, relying on the SSA dominance<br>
property.<br>
Good questions!<br>
<br>
For first one I have added these notes:<br>
[quote]<br>
As follows from FunctionComparator::compare(),<br>
we do CFG<br>
walk, we start<br>
from entry, and then take each terminator. So<br>
it doesn't<br>
matter how in<br>
fact BBs are ordered in function. And since<br>
cmpValues are<br>
called during<br>
this walk, the numbering depends only on how<br>
BBs located<br>
inside the CFG.<br>
[/quote]<br>
<br>
As for second question, I have added next notes:<br>
<br>
[quote]<br>
We also can't use dominance properties.<br>
Consider situation:<br>
If we compare two instruction operands: first<br>
is usage of<br>
local variable<br>
AL from function FL, and second is usage of<br>
local variable<br>
AR from FR.<br>
We are still not able to compare operands of<br>
PHI, since<br>
those could be<br>
operands from further BBs we didn't scan yet.<br>
[/quote]<br>
<br>
You can find comments also in<br>
sn_mapLRComment.txt<br>
I have also described lexicographical ordering<br>
for cmpValues<br>
in method<br>
comments. I have also put them in separated txt<br>
file:<br>
cmpValuesComment.txt.<br>
<br>
> + if (L == R) return 0;<br>
> + // Compare C1 and the bitcast result.<br>
> + if (int Res = cmpConstants(ConstL,<br>
ConstR))<br>
> + return Res;<br>
> [...]<br>
> + return 0;<br>
><br>
> Isn't that just:<br>
> return cmpConstants(ConstL, ConstR);<br>
OOOPS! Fixed.<br>
<br>
I'm working on rest of patches.<br>
Thanks!<br>
<br>
-Stepan.<br>
<br>
Nick Lewycky wrote:<br>
<br>
Stepan Dyatkovskiy wrote:<br>
<br>
Hello Nick!<br>
<br>
Can we proceed to work on patch series?<br>
<br>
<br>
Just in case, I have reattached them to<br>
this post.<br>
Would you prefer to see them in<br>
<a href="http://reviews.llvm.org" target="_blank">http://reviews.llvm.org</a> ?<br>
<br>
<br>
Oddly, I prefer attachments, but we can use<br>
<a href="http://reviews.llvm.org" target="_blank">reviews.llvm.org</a> <<a href="http://reviews.llvm.org" target="_blank">http://reviews.llvm.org</a>><br>
<<a href="http://reviews.llvm.org" target="_blank">http://reviews.llvm.org</a>> if you<br>
prefer?<br>
<br>
0001-constants-comparison.____<u></u>patch<br>
<br>
+ /// 1. Bitcastability.<br>
+ /// Check whether L's type could be<br>
losslessly<br>
bitcasted to R's type.<br>
+ /// On this stage method, in case when<br>
lossless<br>
bitcast is not<br>
possible<br>
+ /// method returns -1 or 1, thus also<br>
defining which<br>
type is<br>
greater in<br>
+ /// context of bitcastability.<br>
<br>
Looking at the code, it looks like the<br>
order is:<br>
1. first class types, by typeid<br>
2. vectors (by element size? by number<br>
of elements?<br>
the two<br>
multiplied together?)<br>
<br>
+ // Nor of TyL and TyR are values of<br>
first class<br>
type. Return<br>
+ // its pointers comparison result.<br>
<br>
"Nor of TyL and TyR" -> "Neither Tyl nor<br>
TyR".<br>
<br>
Optional: "Return its pointers comparison<br>
result" -><br>
"Return the result<br>
of comparing the types"?<br>
<br>
+ if (TyL->getTypeID() ==<br>
Type::X86_MMXTyID)<br>
+ TyLWidth = 64;<br>
+ else if (const VectorType *VecTyL =<br>
dyn_cast<VectorType>(TyL))<br>
+ TyLWidth = VecTyL->getBitWidth();<br>
<br>
Yikes! We never merge function A using a<br>
vector and<br>
function B using an<br>
x86_mmx type together, right? Note what the<br>
LangRef says<br>
about x86_mmx:<br>
"The operations allowed on it are quite<br>
limited:<br>
parameters and return<br>
values, load and store, and bitcast."<br>
<br>
+ case Value::UndefValueVal: return<br>
cmpType(L->getType(),<br>
R->getType());<br>
<br>
You already have L->getType() and<br>
R->getType() in TyL<br>
and TyR. You also<br>
already have cmpType(TyL, TyR) in TypesRes.<br>
Please just<br>
reuse them in<br>
the rest of this function.<br>
<br>
<br>
<br>
0002-values-comparison-____<u></u>replacement-for-enumerate.____<u></u>patch<br>
<br>
+ // Assign serial numbers to values<br>
from left<br>
function, and values<br>
from<br>
+ // right function.<br>
+ // Explanation:<br>
+ // Being comparing functions we need to<br>
compare<br>
values we meet at<br>
left and<br>
+ // right sides.<br>
+ // Its easy to sort things out for<br>
external values.<br>
It just should be<br>
+ // the same value at left and right.<br>
+ // But for local values (those were<br>
introduced inside<br>
function body)<br>
+ // we have to ensure they were<br>
introduced at exactly<br>
the same place,<br>
+ // and plays the same role. How to do it?<br>
+ // Just assign serial number to each<br>
value when we<br>
meet it first<br>
time.<br>
+ // Values that were met at same place<br>
will be with<br>
same serial<br>
numbers.<br>
<br>
This explanation is correct, but I'm<br>
curious whether we<br>
guarantee that<br>
we get the same numbering when comparing<br>
two functions<br>
if I simply<br>
reorder the BasicBlocks in one of the<br>
functions, will we<br>
still assign<br>
the same numberings or not?<br>
<br>
Also, it's not clear from the comment why<br>
we need the<br>
complexity of<br>
numbering them anyways. Why not just start<br>
at the<br>
beginning and run to<br>
the end, relying on the SSA dominance<br>
property to<br>
guarantee that we<br>
visit definitions before uses, and assume<br>
we'll visit<br>
each value in the<br>
same order (look at each instruction and<br>
operand in<br>
order and just<br>
complain about differences)? The answer is<br>
that PHI<br>
nodes mean we will<br>
visit some uses before some definitions.<br>
<br>
Anyways, changing this text is optional in<br>
my mind. I<br>
think it could be<br>
better, but it's not worth spending much<br>
time on it.<br>
<br>
+int<br>
FunctionComparator::cmpValues(<u></u>____const Value *L,<br>
const Value *R) {<br>
<br>
I wouldn't mind an explanation of the<br>
lexographic<br>
ordering on this one<br>
either. Constants before non-constants,<br>
then InlineAsm<br>
(ordered by<br>
address), then visit order.<br>
<br>
+ if (L == R) return 0;<br>
+ // Compare C1 and the bitcast result.<br>
+ if (int Res = cmpConstants(ConstL,<br>
ConstR))<br>
+ return Res;<br>
[...]<br>
+ return 0;<br>
<br>
Isn't that just:<br>
return cmpConstants(ConstL, ConstR);<br>
?<br>
<br>
0003-attributes-comparison.___<u></u>_patch<br>
<br>
+int<br>
FunctionComparator::____<u></u>cmpStrings(StringRef L,<br>
StringRef R) const {<br>
+ // Prevent heavy comparison, compare<br>
sizes first.<br>
+ if (int Res = cmpNumbers(L.size(),<br>
R.size()))<br>
+ return Res;<br>
+<br>
+ return L.compare(R);<br>
+}<br>
<br>
Optional: I'm curious whether that's<br>
actually worth it?<br>
I checked<br>
StringRef and it does the length comparison<br>
last. It<br>
could be that you<br>
know that your strings tend to be longer<br>
than StringRef<br>
which tries to<br>
be efficient in general.<br>
<br>
0004-operations-comparison.___<u></u>_patch<br>
<br>
+int<br>
FunctionComparator::____<u></u>cmpOperation(const<br>
Instruction *L,<br>
<br>
Optional: Again, a description of the<br>
ordering would be<br>
interesting.<br>
Opcode number, then number of operands,<br>
then type, then<br>
'nsw/nuw/exact/volatile'.<br>
<br>
0005-GEP-comparison.patch<br>
<br>
+int FunctionComparator::cmpGEP(___<u></u>_const<br>
GEPOperator<br>
*GEPL,<br>
+ const<br>
GEPOperator<br>
*GEPR) {<br>
<br>
+ unsigned int ASL =<br>
GEPL->getPointerAddressSpace()<u></u>____;<br>
<br>
Extra line after {<br>
<br>
if (DL) {<br>
[...]<br>
+ unsigned BitWidth = DL ?<br>
DL->getPointerSizeInBits(ASL) : 1;<br>
<br>
BitWidth doesn't need to test DL for<br>
null-ness.<br>
<br>
0006-top-level-compare-method.<u></u>____patch<br>
<br>
Perfect!<br>
<br>
0007-sanity-check.patch<br>
<br>
+ // Check symmetricity.<br>
<br>
"symmetricity" -> "symmetry".<br>
<br>
0008-removed-unused-methods.__<u></u>__patch<br>
<br>
Perfect<br>
<br>
0009-FnSet-replaced-with-____<u></u>FnTree.patch<br>
<br>
Okay, I'm confused. We do all this work to<br>
produce a<br>
3-way comparison,<br>
then we wrap it up in an operator< for a<br>
std::set that<br>
will only look at<br>
less-than, but never uses the equals<br>
result. We want to<br>
do one<br>
comparison between two functions, not two<br>
(ie., std::set<br>
does<br>
func1<func2 and func2<func1 to see whether<br>
they're equal<br>
or less than,<br>
and we don't need that since cmp(func1,<br>
func2) tells us<br>
directly). Maybe<br>
this comes in a future patch?<br>
<br>
One other question, I should probably know<br>
the answer to<br>
this but it's<br>
been a while. :) When we start detecting<br>
equal<br>
functions, do we merge<br>
them immediately? If so, can't that cause<br>
functions<br>
already in the set<br>
to have their equality comparisons change<br>
(ie., A calls<br>
X, B calls Y, A<br>
and B are in the set, then we merge X and Y<br>
for being<br>
equal, now we've<br>
changed A and B to both called the same<br>
merged function,<br>
so they start<br>
to compare the same). Sets tend not to like<br>
having their<br>
comparison<br>
functions change on elements that they<br>
contain.<br>
<br>
0010-updated-comments.patch<br>
<br>
Okay.<br>
<br>
0011-removed-DenseMap-helpers.<u></u>____patch<br>
<br>
Yay deleting dead code!<br>
<br>
Nick<br>
<br>
<br>
Thanks!<br>
-Stepan.<br>
<br>
Stepan Dyatkovskiy wrote:<br>
<br>
ping<br>
Stepan Dyatkovskiy wrote:<br>
<br>
Hi Nick,<br>
<br>
Please find the fixed patches<br>
in attachment.<br>
Series starts from "constants<br>
comparison".<br>
<br>
Below small report of what has<br>
been fixed,<br>
and answers on your<br>
questions.<br>
<br>
cmpTypes:<br>
> Please add a comment for<br>
this method.<br>
Include the meaning of the<br>
returned value. ("man strcmp"<br>
for<br>
inspiration.)<br>
Fixed and committed. So you can<br>
look in<br>
trunk, may be I forgot to do<br>
something (hope not :-) )<br>
<br>
checkForLosslessBitcast,<br>
cmpConstants:<br>
> Why isn't this using cmpType?<br>
Fixed.<br>
<br>
> Please put the else on the<br>
same line as<br>
the closing brace.<br>
Fixed.<br>
<br>
>> + else if (const VectorType<br>
*thisPTy =<br>
dyn_cast<VectorType>(L))<br>
> Missing initial capital<br>
Sorry. Fixed. Actually these<br>
typos has been<br>
cloned from<br>
Type::canLosslesslyBitCastTo.<br>
<br>
>> + return<br>
cmpNumbers((uint64_t)L,<br>
(uint64_t)R);<br>
>><br>
> Please assert on unknown<br>
constant.<br>
That's possible. But what if we<br>
really got<br>
unknown constant? New<br>
comming<br>
constant types merely depends on<br>
MergeFunctions implementation.<br>
So we<br>
get crash if it will happen.<br>
And we loose<br>
nothing comparing them<br>
just as<br>
pointers. So, do we really<br>
need an<br>
assertion? Currently I kept it<br>
as it<br>
was. If your answer is "yes, we<br>
need it", it<br>
would easy to add it:<br>
<br>
case Value::FunctionVal:<br>
case Value::GlobalVariableVal:<br>
- case Value::GlobalAliasVal:<br>
- default: // Unknown constant<br>
- return cmpNumbers((uint64_t)L,<br>
(uint64_t)R);<br>
+ case Value::GlobalAliasVal:<br>
+ return cmpNumbers((uint64_t)L,<br>
(uint64_t)R);<br>
+ default:<br>
+ llvm_unreachable("Unknown<br>
constant.");<br>
<br>
About variable names: Var1,<br>
Var2 vs<br>
VarL,VarR.<br>
I think its better to use L/R<br>
concept. Easer<br>
to understand what to<br>
return (-1, or 1) when you see<br>
L/R rather<br>
than Var1/Var2.<br>
Var1/Var2 has been kept for<br>
functions that<br>
are to be removed till the<br>
end of patch series.<br>
F1 and F2 names were also kept<br>
till the "top<br>
level compare method"<br>
patch.<br>
<br>
> Alternatively, any reason<br>
not to have<br>
cmpPointers(const void *L,<br>
const void *R)?<br>
I could do it. I just wanted to<br>
show that<br>
pointers are compared<br>
like a<br>
numbers in some situations.<br>
Actually we do<br>
it in cases when we have<br>
absolutely no idea of smarter<br>
comparison.<br>
And back to cmpPonters. Its<br>
rather about<br>
what intuition tells. If<br>
pointers are equal as numbers,<br>
could they be<br>
different somehow? Could<br>
they be non-castable to numbers<br>
on some<br>
platforms? The last one is<br>
big<br>
trouble for DenseMapInfo<br>
implementation..<br>
If there is any (even very<br>
small)<br>
possibility of such cases - then<br>
yes,<br>
I vote for cmpPointers. The<br>
last word is up<br>
to you anyway.<br>
<br>
Attributes (0005):<br>
> Attributes already have<br>
operator< and<br>
operator==. Please reuse<br>
them.<br>
Fixed. I used simple "if":<br>
<br>
if (LA < RA)<br>
return -1;<br>
if (RA < LA)<br>
return 1;<br>
<br>
Though its possible to use:<br>
<br>
if (int Res = (LA < RA) ? -1 :<br>
(RA < LA) ? 1<br>
: 0)<br>
return Res;<br>
<br>
Which one is more preferable?<br>
<br>
cmpGEP (0007):<br>
> + int cmpGEP(const<br>
GEPOperator *GEP1,<br>
const GEPOperator *GEP2);<br>
> + int cmpGEP(const<br>
GetElementPtrInst<br>
*GEP1,<br>
> + const GetElementPtrInst<br>
*GEP2) {<br>
><br>
> Const members?<br>
We can't make it constant,<br>
since it compares<br>
values (cmpValues, aka<br>
enumerate). So, if we see Value<br>
first time,<br>
we have to add it into<br>
sn_mapL/R.<br>
<br>
> I think you should put this<br>
under if (DL)<br>
{ /* declare Offset1,<br>
Offset2, etc. */ }<br>
Fixed.<br>
<br>
About 0008:<br>
> Did you mean "cmpOperation"?<br>
Yes. We could keep it in one<br>
place. I have<br>
fixed this case and<br>
removed<br>
TODO.<br>
<br>
> "compare" still returns<br>
"bool". I'm going<br>
to assume this was<br>
meant to<br>
go in 0009.<br>
Yes.<br>
<br>
About 0011:<br>
> std::set is frowned upon, see<br>
<br>
<a href="http://llvm.org/docs/____ProgrammersManual.html#set" target="_blank">http://llvm.org/docs/____<u></u>ProgrammersManual.html#set</a><br>
<<a href="http://llvm.org/docs/__ProgrammersManual.html#set" target="_blank">http://llvm.org/docs/__<u></u>ProgrammersManual.html#set</a>><br>
<br>
<<a href="http://llvm.org/docs/__ProgrammersManual.html#set" target="_blank">http://llvm.org/docs/__<u></u>ProgrammersManual.html#set</a><br>
<<a href="http://llvm.org/docs/ProgrammersManual.html#set" target="_blank">http://llvm.org/docs/<u></u>ProgrammersManual.html#set</a>>><br>
. Do you actually<br>
rely<br>
on the stable iterator<br>
guarantee? Or would<br>
another set-like container<br>
be a<br>
better fit?<br>
Huh.. I have looked for<br>
alternatives.<br>
Unfortunately SmallSet is not<br>
suitable, since we need to<br>
lookup existing<br>
items, and SmallSet::find<br>
method is not present. May be I<br>
have missed<br>
something.<br>
Actually we need binary tree<br>
implementation.<br>
Do we have better<br>
analogue?<br>
I could implement new one.<br>
Think it should<br>
be just one more patch<br>
afterwards.<br>
<br>
>No. An identifier with<br>
underscore then<br>
capital letter is reserved<br>
for<br>
the implementation. You can<br>
just call them<br>
"F" and "DL", then the<br>
ctor<br>
initializer list can be "F(F),<br>
DL(DL)" and<br>
that will work fine.<br>
Fixed.<br>
<br>
0013:<br>
> Sorry, I'm not sure what<br>
this sentence<br>
means? The thing your<br>
example<br>
is talking about is showing is<br>
that two<br>
functions in an SCC may be<br>
equivalent, and detecting them<br>
requires<br>
"blinding" (ignoring) certain<br>
details of the function when<br>
you do the<br>
comparison. We blind<br>
ourselves<br>
to the pointee types, but not<br>
to callees of<br>
functions in the same<br>
SCC.<br>
<br>
I meant generalising of<br>
cross-reference case:<br>
in old implementation, while<br>
comparing F and<br>
G functions, we treat as<br>
equal case when F uses G, with<br>
case when G<br>
uses F (at the same<br>
place).<br>
It could happen that F uses G1,<br>
while G1<br>
uses G2, while G2 uses F.<br>
So it<br>
still the same. But currently<br>
we have to<br>
invent way how to detect<br>
such<br>
cases.<br>
<br>
Thanks for reviews!<br>
-Stepan<br>
<br>
Stepan Dyatkovskiy wrote:<br>
<br>
Hi Nick,<br>
I have committed 0001 as<br>
r203788.<br>
I'm working on fixes for<br>
0002 - 0014.<br>
<br>
> After reading through<br>
this patch<br>
series, I feel like I'm<br>
missing<br>
> something important.<br>
Where's the sort<br>
function? It looks like<br>
we're<br>
> still comparing all<br>
functions to all<br>
other functions.<br>
When you insert functions<br>
into std::set<br>
or its analo g s it does all<br>
the<br>
job for you. Since<br>
internally it builds<br>
a binary tree using "less"<br>
comparison, and each<br>
insert/look-up<br>
operation takes O(log(N))<br>
time.<br>
<br>
-Stepan.<br>
<br>
Nick Lewycky wrote:<br>
<br>
On 27 February 2014<br>
08:23, Stepan<br>
Dyatkovskiy<br>
<<a href="mailto:stpworld@narod.ru" target="_blank">stpworld@narod.ru</a> <mailto:<a href="mailto:stpworld@narod.ru" target="_blank">stpworld@narod.ru</a>><br>
<br>
<mailto:<a href="mailto:stpworld@narod.ru" target="_blank">stpworld@narod.ru</a> <mailto:<a href="mailto:stpworld@narod.ru" target="_blank">stpworld@narod.ru</a>>><br>
<br>
<mailto:<a href="mailto:stpworld@narod.ru" target="_blank">stpworld@narod.ru</a> <mailto:<a href="mailto:stpworld@narod.ru" target="_blank">stpworld@narod.ru</a>><br>
<br>
<mailto:<a href="mailto:stpworld@narod.ru" target="_blank">stpworld@narod.ru</a> <mailto:<a href="mailto:stpworld@narod.ru" target="_blank">stpworld@narod.ru</a>>>>><br>
wrote:<br>
<br>
Hi Nick,<br>
<br>
I tried to rework</blockquote></blockquote>
</blockquote></div><br></div></div>