<html><head><meta http-equiv="Content-Type" content="text/html charset=utf-8"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class=""><div><blockquote type="cite" class=""><div class="">On Aug 24, 2017, at 3:01 AM, Frozen <<a href="mailto:bluechristlove@163.com" class="">bluechristlove@163.com</a>> wrote:</div><div class=""><div style="font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; line-height: 1.7; font-size: 14px; font-family: Arial;" class="">Firstly, I briefly clarify the logic of Info.noteSideEffect. Info.noteSideEffect will set <font face="arial" class=""><span style="white-space: pre-wrap;" class="">EvalStatus.HasSideEffects = true; and make </span></font><span style="font-family: arial; white-space: pre-wrap;" class="">hasUnacceptableSideEffect function always return true so that our </span><div style="margin-top: 0px; margin-bottom: 0px;" class="">EvaluateAsInt alway return false when LHS can not evaulate.</div></div></div></blockquote><div><br class=""></div>Yes, I understand the implementation.</div><div><br class=""></div><div><blockquote type="cite" class=""><div class=""><div style="font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; line-height: 1.7; font-size: 14px; font-family: Arial;" class=""><div class="">Then I want to express one significant point of it, that is debug. Let us compare it with GCC in the following example.<br class=""><br class=""><div class="">int main()</div><div class="">{</div><div class=""> int x;</div><div class=""> if (1 || x) {</div><div class=""> printf("LHS\n");</div><div class=""> }</div><div class=""><br class=""></div><div class=""> if (x || 1) {</div><div class=""> printf("RHS\n");</div><div class=""> }</div><div class="">}<br class=""><br class="">When we use GCC to debug it, we will get the following result:<br class=""><br class=""><div class="">(gdb) b main</div><div class="">Breakpoint 1 at 0x10000620: file a.c, line 5.</div><div class="">(gdb) r</div><div class="">Starting program: a.out</div><div class=""><br class=""></div><div class="">Breakpoint 1, main () at a.c:5</div><div class="">5<span style="white-space: pre;" class=""> </span><span class="Apple-converted-space"> </span> printf("LHS\n");</div><div class="">Missing separate debuginfos, use: debuginfo-install glibc-2.17-78.ael7b.ppc64le</div><div class="">(gdb) step</div><div class="">LHS</div><div class="">9<span style="white-space: pre;" class=""> </span><span class="Apple-converted-space"> </span> printf("RHS\n");</div><div class="">(gdb) step</div><div class="">RHS</div><div class="">11<span style="white-space: pre;" class=""> </span>}</div></div></div><br class=""><div class="">And if we use clang, we will get:<br class=""><div class="">(gdb) b main</div><div class="">Breakpoint 1 at 0x10000614: file a.c, line 5.</div><div class="">(gdb) r</div><div class="">Starting program: a.out</div><div class=""><br class=""></div><div class="">Breakpoint 1, main () at a.c:5</div><div class="">5<span style="white-space: pre;" class=""> </span><span class="Apple-converted-space"> </span> printf("LHS\n");</div><div class="">Missing separate debuginfos, use: debuginfo-install glibc-2.17-78.ael7b.ppc64le</div><div class="">(gdb) step</div><div class="">LHS</div><div class="">8<span style="white-space: pre;" class=""> </span><span class="Apple-converted-space"> </span> if (x || 1) {</div><div class="">(gdb) step</div><div class="">9<span style="white-space: pre;" class=""> </span><span class="Apple-converted-space"> </span> printf("RHS\n");</div><div class="">(gdb) step</div><div class="">RHS</div><div class="">11<span style="white-space: pre;" class=""> </span>}</div></div><div class=""><br class="">We can see that we will stop at line 8 and can not stop at line 9 directly like GCC or LHS is constant. This is somehow not unified behaviour and make uses feel strange.</div></div></div></blockquote><div><br class=""></div>Clang's behavior seems correct. The general goal of stepping is to stop at every statement boundary.</div><div><br class=""></div><div>John.</div><div><br class=""><blockquote type="cite" class=""><div class=""><div style="font-style: normal; font-variant-caps: normal; font-weight: normal; letter-spacing: normal; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; word-spacing: 0px; -webkit-text-stroke-width: 0px; line-height: 1.7; font-size: 14px; font-family: Arial;" class=""><div style="position: relative; zoom: 1;" class=""></div><div id="divNeteaseMailCard" class=""></div><br class="">在 2017-08-24 13:59:06，"John McCall" <<a href="mailto:rjmccall@apple.com" class="">rjmccall@apple.com</a>> 写道：<br class=""><blockquote id="isReplyContent" style="padding-left: 1ex; margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204);" class=""><div class=""><blockquote type="cite" class=""><div class="">On Aug 24, 2017, at 1:26 AM, Frozen <<a href="mailto:bluechristlove@163.com" class="">bluechristlove@163.com</a>> wrote:</div><div class=""><div class="" style="line-height: 1.7; font-size: 14px; font-family: Arial;"><div class="">Not. Original implementation used the default parameter value, i.e. No Side Effects<br class=""><div class=""><br class=""></div><div class=""> /// EvaluateAsInt - Return true if this is a constant which we can fold and</div><div class=""> /// convert to an integer, using any crazy technique that we want to.</div><div class=""> bool EvaluateAsInt(llvm::APSInt &Result, const ASTContext &Ctx,</div><div class=""> <b class="">SideEffectsKind AllowSideEffects = SE_NoSideEffects</b>) const;<br class=""><br class="">But, now, we should<span class="Apple-converted-space"> </span><b class="">allow side effects</b><span class="Apple-converted-space"> </span>and pass the computation to the function <b class="" style="font-family: 'PingFang SC'; font-size: 10.5pt;">Info.noteSideEffect().<span class="Apple-converted-space"> </span></b>As the comment of source code said:</div></div><div class=""><div class=""><br class=""></div><div class="">bool DataRecursiveIntBinOpEvaluator::</div><div class=""> VisitBinOpLHSOnly(EvalResult &LHSResult, const BinaryOperator *E,</div><div class=""> bool &SuppressRHSDiags) {</div><div class=""> if (E->isLogicalOp()) {</div><div class=""> bool LHSAsBool;</div><div class=""> if (!LHSResult.Failed && HandleConversionToBool(LHSResult.Val, LHSAsBool)) {</div><div class=""> // We were able to evaluate the LHS, see if we can get away with not</div><div class=""> // evaluating the RHS: 0 && X -> 0, 1 || X -> 1</div><div class=""> if (LHSAsBool == (E->getOpcode() == BO_LOr)) {</div><div class=""> Success(LHSAsBool, E, LHSResult.Val);</div><div class=""> return false; // Ignore RHS</div><div class=""> }</div><div class=""> } else {</div><div class=""> LHSResult.Failed = true;</div><div class=""><br class=""></div><div class=""><b class=""> // Since we weren't able to evaluate the left hand side, it</b></div><div class=""><b class=""> // might have had side effects.</b></div><div class=""><b class=""> if (!Info.noteSideEffect())</b></div><div class=""><b class=""> return false;</b></div><div class=""><br class=""></div><div class=""> // We can't evaluate the LHS; however, sometimes the result</div><div class=""> // is determined by the RHS: X && 0 -> 0, X || 1 -> 1.</div><div class=""> // Don't ignore RHS and suppress diagnostics from this arm.</div><div class=""> SuppressRHSDiags = true;</div><div class=""> }</div><div class=""><br class=""></div><div class=""> return true;</div><div class=""> }</div></div><div class=""><br class=""></div><div class="">So we might have had side effects when constant-evaluation in if stmt condition , so we should use <b class="" style="font-family: 'PingFang SC'; font-size: 10.5pt;">Expr::SE_AllowSideEffects,<span class="Apple-converted-space"> </span></b>then use <b class="">Info.noteSideEffect()<span class="Apple-converted-space"> </span></b>to compute the result. </div></div></div></blockquote><div class=""><br class=""></div>Info.noteSideEffect() is not given the expression to evaluate, nor does it call back to IR-generation in a way that would allow us to emit the side effect. A more promising approach would be to modify IRGen's logic for emitting branches on conditions, which is already a special-case expression emitter due to the desire to directly branch on && and || instead of branching on a phi. That emitter has special-case logic for constant branches, but from the generated code it looks like we're not applying that in all positions, just at the top level. Recognizing when we're about to branch on a constant i1 would be sufficient to catch things like this and allow IRGen to reap the (small) benefits of emitting less silly code. I don't think there's much point in trying to recognize (X || true) especially, but we could do that, too, if you have a good argument.</div><div class=""><br class=""></div><div class="">John.</div><div class=""><br class=""><blockquote type="cite" class=""><div class=""><div class="" style="line-height: 1.7; font-size: 14px; font-family: Arial;"><br class=""><div class="" style="position: relative; zoom: 1;"></div><div class=""></div><br class="">在 2017-08-24 02:02:23，"John McCall" <<a href="mailto:rjmccall@apple.com" class="">rjmccall@apple.com</a>> 写道：<br class=""><blockquote id="isReplyContent" class="" style="padding-left: 1ex; margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-style: solid; border-left-color: rgb(204, 204, 204);"><br class=""><div class=""><blockquote type="cite" class=""><div class="">On Aug 23, 2017, at 1:58 PM, bluechristlove <<a href="mailto:bluechristlove@163.com" class="">bluechristlove@163.com</a>> wrote:</div><br class="Apple-interchange-newline"><div class=""><div class=""><div class="" style="font-family: 'PingFang SC'; font-size: 10.5pt;">This is not specific example. If we don’t fix it, user will feel strange why LHS of if stmt condition could do constant folding but RHS couldn’t.<div class=""><br class=""></div><div class="">From the source code view’s point, this is our compiler bug.</div></div></div></div></blockquote><blockquote type="cite" class=""><div class=""><div class=""><div class="" style="font-family: 'PingFang SC'; font-size: 10.5pt;"><div class=""><br class=""></div><div class="">In the ExprConstant.cpp:</div><div class=""><br class=""></div><div class=""><div class=""> case Job::BinOpKind: {</div><div class=""> ...</div><div class=""> if (!VisitBinOpLHSOnly(Result, Bop, SuppressRHSDiags)) {</div><div class=""> Queue.pop_back();</div><div class=""> return;</div><div class=""> }</div><div class=""> ...</div><div class=""> job.Kind = Job::BinOpVisitedLHSKind;</div><div class=""> enqueue(Bop->getRHS());</div><div class=""> return;</div><div class=""> }</div><div class=""> </div><div class=""> case Job::BinOpVisitedLHSKind: {</div><div class=""> ...</div><div class=""> Result.Failed = !VisitBinOp(job.LHSResult, RHS, Bop, Result.Val);</div><div class=""> Queue.pop_back();</div><div class=""> return;</div><div class=""> }</div><div class=""><br class=""></div><div class="">If we can not evaluate LHS successfully, we will evaluate RHS (function VisitBinOp). In the function VisitBinOp:</div><div class=""><br class=""></div><div class=""><div class=""> if (E->isLogicalOp()) {</div><div class=""> bool lhsResult, rhsResult;</div><div class=""> bool LHSIsOK = HandleConversionToBool(LHSResult.Val, lhsResult);</div><div class=""> bool RHSIsOK = HandleConversionToBool(RHSResult.Val, rhsResult);</div><div class=""> if (LHSIsOK) {</div><div class=""> if (RHSIsOK) {</div><div class=""> if (E->getOpcode() == BO_LOr)</div><div class=""> return Success(lhsResult || rhsResult, E, Result);</div><div class=""> else</div><div class=""> return Success(lhsResult && rhsResult, E, Result);</div><div class=""> }</div><div class=""> } else {</div><div class=""> if (RHSIsOK) {</div><div class=""><b class=""> // We can't evaluate the LHS; however, sometimes the result</b></div><div class=""><b class=""> // is determined by the RHS: X && 0 -> 0, X || 1 -> 1.</b></div><div class=""> if (rhsResult == (E->getOpcode() == BO_LOr)) {</div><div class=""> return Success(rhsResult, E, Result);</div><div class=""> }</div><div class=""> }</div><div class=""> }</div></div><div class=""><br class=""></div><div class="">We can see that our previous implementation has consider this condition of RHS constant folding. But because of ignore the parameter Expr::SE_AllowSideEffects in the function <span class="" style="font-size: 10.5pt;">ConstantFoldsToSimpleInteger</span><span class="" style="font-size: 10.5pt;">:</span></div><div class=""><br class=""></div><div class=""><div class="">bool CodeGenFunction::ConstantFoldsToSimpleInteger(const Expr *Cond,</div><div class=""> llvm::APSInt &ResultInt,</div><div class=""> bool AllowLabels) {</div><div class=""> // FIXME: Rename and handle conversion of other evaluatable things</div><div class=""> // to bool.</div><div class=""> llvm::APSInt Int;</div><div class=""> if (!Cond->EvaluateAsInt(Int, getContext(),<span class="Apple-converted-space"> </span><b class="">Expr::SE_AllowSideEffects)</b>) -------> allow side effect</div><div class=""> return false; // Not foldable, not integer or not fully evaluatable.</div></div><div class=""><br class=""></div><div class="">We will not make the RHS constant folding happened.</div><div class=""><br class=""></div><div class=""><div class="">bool DataRecursiveIntBinOpEvaluator::</div><div class=""> VisitBinOpLHSOnly(EvalResult &LHSResult, const BinaryOperator *E,</div><div class=""> bool &SuppressRHSDiags) {</div><div class="">….</div><div class=""><br class=""></div><div class=""> if (E->isLogicalOp()) {</div><div class=""> bool LHSAsBool;</div><div class=""><span class="" style="font-size: 10.5pt;"> E->getLHS()->dumpColor();</span></div><div class=""> if (!LHSResult.Failed && HandleConversionToBool(LHSResult.Val, LHSAsBool)) {</div><div class=""> // We were able to evaluate the LHS, see if we can get away with not</div><div class=""> // evaluating the RHS: 0 && X -> 0, 1 || X -> 1</div><div class=""> if (LHSAsBool == (E->getOpcode() == BO_LOr)) {</div><div class=""> Success(LHSAsBool, E, LHSResult.Val);</div><div class=""> return false; // Ignore RHS</div><div class=""> }</div><div class=""> } else {</div><div class=""> LHSResult.Failed = true;</div><div class=""><br class=""></div><div class=""> // Since we weren't able to evaluate the left hand side, it</div><div class=""> // might have had side effects.</div><div class=""><b class=""> if (!Info.noteSideEffect()) { --------> HERE!!!</b></div><div class=""> return false;</div><div class=""> }</div></div><div class="">The detail source code analysis could be find in the comment 2 of <a href="https://bugs.llvm.org/show_bug.cgi?id=34229" class="" style="font-family: Arial; font-size: 10.5pt;">https://bugs.llvm.org/show_bug.cgi?id=34229</a></div><div class=""><br class=""></div><div class="">So, I think we should fix it. <br class=""></div></div></div></div></div></blockquote><div class=""><br class=""></div>Are you arguing that we should generally ignore side-effects when constant-evaluating an if condition?</div><div class=""><br class=""></div><div class="">John.</div><div class=""><br class=""><blockquote type="cite" class=""><div class=""><div class=""><div class="" style="font-family: 'PingFang SC'; font-size: 10.5pt;"><div class=""><div class=""><div class="yomail-sig" style="margin: 0px; padding: 0px;"><div class="yomail-sig" style="padding: 0px 0px 0px 20px; margin: 0px 0px 0px -20px;"><span class=""></span><div class="" style="font-family: 'PingFang SC'; font-size: 10.5pt;"><br class=""></div></div></div><br class=""><div class="-eMc-quote" style="margin: 0px 0px 0px 0.8ex; display: block;"><div class="" style="border-style: solid none none; border-top-width: 1pt; border-top-color: rgb(181, 196, 223); padding: 3pt 0cm 0cm;"><div class="" style="padding: 8px; font-size: 12px; background-color: rgb(239, 239, 239);"><div class=""><b class="">发件人：</b> <a href="mailto:rjmccall@apple.com" target="_blank" class="">John McCall</a></div><div class=""><b class="">发送时间：</b> 2017-08-23 01:30:45</div><div class=""><b class="">收件人：</b> <span class="Apple-converted-space"> </span><a href="mailto:bluechristlove@163.com" target="_blank" class="">Frozen</a></div><div class=""><b class="">抄送：</b> <span class="Apple-converted-space"> </span><a href="mailto:cfe-dev@lists.llvm.org" target="_blank" class="">cfe-dev@lists.llvm.org</a></div><div class=""><b class="">主题：</b> Re: [cfe-dev] Evaluate constant condition of if statement has no effect</div></div></div><blockquote class="" style="margin: 0px;"><div dir="ltr" class=""><div class=""><div class=""></div><blockquote type="cite" class=""><div class="">On Aug 21, 2017, at 12:28 AM, Frozen via cfe-dev <<a href="mailto:cfe-dev@lists.llvm.org" class="">cfe-dev@lists.llvm.org</a>> wrote:</div><div class=""><div class="" style="line-height: 1.7; font-size: 14px; font-family: Arial;"><div class="">Simple Example:<br class=""><br class="">int main()</div><div class="">{</div><div class=""> int x;</div><div class=""> if (x || 1) {}</div><div class="">}</div><div class=""><br class=""></div><div class="">Clang can not evaluate this condition and will emit IR like this:</div><div class=""><br class=""></div><div class="">define signext i32 @main() #0 {</div><div class="">entry:</div><div class=""> %retval = alloca i32, align 4</div><div class=""> %x = alloca i32, align 4</div><div class=""> store i32 0, i32* %retval, align 4</div><div class=""> %0 = load i32, i32* %x, align 4</div><div class=""> %tobool = icmp ne i32 %0, 0</div><div class=""> br i1 %tobool, label %if.then, label %lor.lhs.false</div><div class=""><br class=""></div><div class="">lor.lhs.false: ; preds = %entry</div><div class=""> br i1 true, label %if.then, label %if.end</div><div class=""><br class=""></div><div class="">if.then: ; preds = %lor.lhs.false, %entry</div><div class=""> br label %if.end</div><div class=""><br class=""></div><div class="">if.end: ; preds = %if.then, %lor.lhs.false</div><div class=""> %1 = load i32, i32* %retval, align 4</div><div class=""> ret i32 %1</div><div class="">}</div><div class=""><br class=""></div><div class="">However, when we swap the position of LHS and RHS(i.e. if (1 || x), Clang can recognize it:</div><div class=""><br class=""></div><div class="">define signext i32 @main() #0 {</div><div class="">entry:</div><div class=""> %x = alloca i32, align 4</div><div class=""> ret i32 0</div><div class="">}<br class=""><br class="">I also find the root issue and propose one potential solution in comment 2 of this link: <a href="https://bugs.llvm.org/show_bug.cgi?id=34229" class="">https://bugs.llvm.org/show_bug.cgi?id=34229</a><br class=""><br class="">Any idea?<br class=""></div></div></div></blockquote><div class=""><br class=""></div>There will always be some example of something that we generate less efficiently at -O0 than we could. Why is this example specifically worth optimizing?</div><div class=""><br class=""></div><div class="">John.</div></div></blockquote></div></div></div></div></div></div></blockquote></div></blockquote></div></div></blockquote></div></blockquote></div></div></blockquote></div><br class=""></body></html>