[cfe-dev] Evaluate constant condition of if statement has no effect

[John McCall via cfe-dev]

> On Aug 23, 2017, at 1:58 PM, bluechristlove <bluechristlove at 163.com> wrote:
> 
> 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.
> 
> From the source code view’s point, this is our compiler bug.
> 
> In the ExprConstant.cpp:
> 
>     case Job::BinOpKind: {
>       ...
>       if (!VisitBinOpLHSOnly(Result, Bop, SuppressRHSDiags)) {
>         Queue.pop_back();
>         return;
>       }
>       ...
>       job.Kind = Job::BinOpVisitedLHSKind;
>       enqueue(Bop->getRHS());
>       return;
>     }
>       
>     case Job::BinOpVisitedLHSKind: {
>       ...
>       Result.Failed = !VisitBinOp(job.LHSResult, RHS, Bop, Result.Val);
>       Queue.pop_back();
>       return;
>     }
> 
> If we can not evaluate LHS successfully, we will evaluate RHS (function VisitBinOp). In the function VisitBinOp:
> 
>   if (E->isLogicalOp()) {
>     bool lhsResult, rhsResult;
>     bool LHSIsOK = HandleConversionToBool(LHSResult.Val, lhsResult);
>     bool RHSIsOK = HandleConversionToBool(RHSResult.Val, rhsResult);
>     if (LHSIsOK) {
>       if (RHSIsOK) {
>         if (E->getOpcode() == BO_LOr)
>           return Success(lhsResult || rhsResult, E, Result);
>         else
>           return Success(lhsResult && rhsResult, E, Result);
>       }
>     } else {
>       if (RHSIsOK) {
>         // We can't evaluate the LHS; however, sometimes the result
>         // is determined by the RHS: X && 0 -> 0, X || 1 -> 1.
>         if (rhsResult == (E->getOpcode() == BO_LOr)) {
>           return Success(rhsResult, E, Result);
>         }
>       }
>     }
> 
> 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 ConstantFoldsToSimpleInteger:
> 
> bool CodeGenFunction::ConstantFoldsToSimpleInteger(const Expr *Cond,
>                                                    llvm::APSInt &ResultInt,
>                                                    bool AllowLabels) {
>   // FIXME: Rename and handle conversion of other evaluatable things
>   // to bool.
>   llvm::APSInt Int;
>   if (!Cond->EvaluateAsInt(Int, getContext(), Expr::SE_AllowSideEffects)) -------> allow side effect
>     return false;  // Not foldable, not integer or not fully evaluatable.
> 
> We will not make the RHS constant folding happened.
> 
> bool DataRecursiveIntBinOpEvaluator::
>        VisitBinOpLHSOnly(EvalResult &LHSResult, const BinaryOperator *E,
>                          bool &SuppressRHSDiags) {
> ….
> 
>   if (E->isLogicalOp()) {
>     bool LHSAsBool;
>     E->getLHS()->dumpColor();
>     if (!LHSResult.Failed && HandleConversionToBool(LHSResult.Val, LHSAsBool)) {
>       // We were able to evaluate the LHS, see if we can get away with not
>       // evaluating the RHS: 0 && X -> 0, 1 || X -> 1
>       if (LHSAsBool == (E->getOpcode() == BO_LOr)) {
>         Success(LHSAsBool, E, LHSResult.Val);
>         return false; // Ignore RHS
>       }
>     } else {
>       LHSResult.Failed = true;
> 
>      // Since we weren't able to evaluate the left hand side, it
>       // might have had side effects.
>       if (!Info.noteSideEffect()) { --------> HERE!!!
>         return false;
>       }
> The detail source code analysis could be find in the comment 2 of https://bugs.llvm.org/show_bug.cgi?id=34229 <https://bugs.llvm.org/show_bug.cgi?id=34229>
> 
> So, I think we should fix it. 

Are you arguing that we should generally ignore side-effects when constant-evaluating an if condition?

John.

> 
> 
> 发件人： John McCall <mailto:rjmccall at apple.com>
> 发送时间： 2017-08-23 01:30:45
> 收件人：  Frozen <mailto:bluechristlove at 163.com>
> 抄送：  cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>
> 主题： Re: [cfe-dev] Evaluate constant condition of if statement has no effect
>> On Aug 21, 2017, at 12:28 AM, Frozen via cfe-dev <cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>> wrote:
>> Simple Example:
>> 
>> int main()
>> {
>>   int x;
>>   if (x || 1) {}
>> }
>> 
>> Clang can not evaluate this condition and will emit IR like this:
>> 
>> define signext i32 @main() #0 {
>> entry:
>>   %retval = alloca i32, align 4
>>   %x = alloca i32, align 4
>>   store i32 0, i32* %retval, align 4
>>   %0 = load i32, i32* %x, align 4
>>   %tobool = icmp ne i32 %0, 0
>>   br i1 %tobool, label %if.then, label %lor.lhs.false
>> 
>> lor.lhs.false:                                    ; preds = %entry
>>   br i1 true, label %if.then, label %if.end
>> 
>> if.then:                                          ; preds = %lor.lhs.false, %entry
>>   br label %if.end
>> 
>> if.end:                                           ; preds = %if.then, %lor.lhs.false
>>   %1 = load i32, i32* %retval, align 4
>>   ret i32 %1
>> }
>> 
>> However, when we swap the position of LHS and RHS(i.e. if (1 || x), Clang can recognize it:
>> 
>> define signext i32 @main() #0 {
>> entry:
>>   %x = alloca i32, align 4
>>   ret i32 0
>> }
>> 
>> I also find the root issue and propose one potential solution in comment 2 of this link: https://bugs.llvm.org/show_bug.cgi?id=34229 <https://bugs.llvm.org/show_bug.cgi?id=34229>
>> 
>> Any idea?
> 
> 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?
> 
> John.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20170823/1f8a77c9/attachment.html>