<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p><br>
</p>
<div class="moz-cite-prefix">On 1/2/22 9:49 AM, Nuno Lopes wrote:<br>
</div>
<blockquote type="cite"
cite="mid:ED4E860AB90D49A5A08B68865447FF1D@PC07655">That's a long
story..
<br>
<br>
The return value of InstSimplify is only guaranteed to be a
*refinement* of the input, meaning it's not necessarily
equivalent. This is perfectly fine. LLVM can transform undef ->
0; it removes behavior.
<br>
<br>
GVN algorithms, on the other hand, compute classes of equivalent
expressions. But NewGVN uses InstSimplify to canonicalize
expressions before computing the equivalence classes. But, ouch,
there's a mismatch: GVN deals with equivalences, while
InstSimplify gives it refinements.
<br>
<br>
A workaround implemented at the time was to disable some
refinements from InstSimplify by disabling the matching of undef.
This is not a complete solution, but a band aid (in theory, at
least, I didn't fuzz it to check if it's possible to get a
miscompilation in practice).
<br>
See <a class="moz-txt-link-freetext" href="https://llvm.org/doxygen/structllvm_1_1SimplifyQuery.html">https://llvm.org/doxygen/structllvm_1_1SimplifyQuery.html</a>
<br>
</blockquote>
<p>Ah, gotcha. The refinement vs equivalence point makes sense. <br>
</p>
<p>A couple of suggestions/ideas:</p>
<ul>
<li>The naming in the code should match the refinement
terminology. AllowRefinement would be a much clearer name than
WithoutUndef. In particular, the "isUndefValue" in the code is
confusing.<br>
</li>
<li>We really need a way to test the logic and fuzz it. As a
suggestion, maybe add an off-by-default option to the
instsimplify pass called "-instsimplify-disallow-refinement",
and write LIT cases for the interesting differences? <br>
</li>
<li>Once we had that, using alive2 to check fuzz outputs for
unexpected refinements would be pretty straight forward. <br>
</li>
</ul>
<blockquote type="cite"
cite="mid:ED4E860AB90D49A5A08B68865447FF1D@PC07655">
<br>
Note that InstSimplify does have transformations that are wrong in
the presence of undef. It applies redistributivity freely, which
is not guaranteed to yield a correct result until we kill undef.
But again, it disables undef matching when it recurses, so maybe
it's ok in practice too (candidate for fuzzing too).
<br>
</blockquote>
And a great case to cover with some LIT tests. :)<br>
<blockquote type="cite"
cite="mid:ED4E860AB90D49A5A08B68865447FF1D@PC07655">
<br>
Nuno
<br>
<br>
<br>
-----Original Message----- From: Philip Reames
<br>
Sent: Sunday, January 2, 2022 5:28 PM
<br>
Subject: Re: [llvm] 64af9f6 - [InstSimplify] add 'x + poison ->
poison' (needed for NewGVN)
<br>
<br>
Er, that really stinks that the public API of this utility is
broken then.
<br>
<br>
Can you explain the context of why newgvn needs to "disable
matching on
<br>
undef" and instsimplify is fine to run without doing so? If
there's a
<br>
clear comment in the code already, a pointer to that is fine.
<br>
<br>
Philip
<br>
<br>
On 1/2/22 9:26 AM, Nuno Lopes wrote:
<br>
<blockquote type="cite">I don't think it can be reproduced with
InstSimplify alone. InstSimplify already simplified the test
case I added. NewGVN disables matching of undef, which
InstSimplify does not.
<br>
<br>
Nuno
<br>
<br>
<br>
-----Original Message-----
<br>
From: Philip Reames
<br>
Sent: Sunday, January 2, 2022 5:10 PM
<br>
Subject: Re: [llvm] 64af9f6 - [InstSimplify] add 'x + poison
-> poison' (needed for NewGVN)
<br>
<br>
I agree, this really needs a test. I see you later added a GVN
test for
<br>
the same, can you add an instsimplify one please?
<br>
<br>
Philip
<br>
<br>
On 12/30/21 4:02 AM, Roman Lebedev via llvm-commits wrote:
<br>
<blockquote type="cite">Test?
<br>
<br>
On Thu, Dec 30, 2021 at 2:53 PM Nuno Lopes via llvm-commits
<br>
<a class="moz-txt-link-rfc2396E" href="mailto:llvm-commits@lists.llvm.org"><llvm-commits@lists.llvm.org></a> wrote:
<br>
<blockquote type="cite">
<br>
Author: Nuno Lopes
<br>
Date: 2021-12-30T11:52:42Z
<br>
New Revision: 64af9f61c30191482979c6883e4cc63703f12010
<br>
<br>
URL:
<a class="moz-txt-link-freetext" href="https://github.com/llvm/llvm-project/commit/64af9f61c30191482979c6883e4cc63703f12010">https://github.com/llvm/llvm-project/commit/64af9f61c30191482979c6883e4cc63703f12010</a><br>
DIFF:
<a class="moz-txt-link-freetext" href="https://github.com/llvm/llvm-project/commit/64af9f61c30191482979c6883e4cc63703f12010.diff">https://github.com/llvm/llvm-project/commit/64af9f61c30191482979c6883e4cc63703f12010.diff</a><br>
<br>
LOG: [InstSimplify] add 'x + poison -> poison' (needed
for NewGVN)
<br>
<br>
Added:
<br>
<br>
<br>
Modified:
<br>
llvm/lib/Analysis/InstructionSimplify.cpp
<br>
<br>
Removed:
<br>
<br>
<br>
<br>
################################################################################
<br>
diff --git a/llvm/lib/Analysis/InstructionSimplify.cpp
b/llvm/lib/Analysis/InstructionSimplify.cpp
<br>
index 1c26ab3619089..4a8dc754349bd 100644
<br>
--- a/llvm/lib/Analysis/InstructionSimplify.cpp
<br>
+++ b/llvm/lib/Analysis/InstructionSimplify.cpp
<br>
@@ -620,6 +620,10 @@ static Value *SimplifyAddInst(Value
*Op0, Value *Op1, bool IsNSW, bool IsNUW,
<br>
if (Constant *C =
foldOrCommuteConstant(Instruction::Add, Op0, Op1, Q))
<br>
return C;
<br>
<br>
+ // X + poison -> poison
<br>
+ if (isa<PoisonValue>(Op1))
<br>
+ return Op1;
<br>
+
<br>
// X + undef -> undef
<br>
if (Q.isUndefValue(Op1))
<br>
return Op1;
<br>
<br>
<br>
<br>
_______________________________________________
<br>
llvm-commits mailing list
<br>
<a class="moz-txt-link-abbreviated" href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>
<br>
<a class="moz-txt-link-freetext" href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a>
<br>
</blockquote>
_______________________________________________
<br>
llvm-commits mailing list
<br>
<a class="moz-txt-link-abbreviated" href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>
<br>
<a class="moz-txt-link-freetext" href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a>
<br>
</blockquote>
<br>
</blockquote>
<br>
</blockquote>
</body>
</html>