Skip to content

Miscompilation with libstdc++'s std::optional<int> and -O1 #98753

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
cadubentzen opened this issue Jul 13, 2024 · 6 comments · Fixed by #98898
Closed

Miscompilation with libstdc++'s std::optional<int> and -O1 #98753

cadubentzen opened this issue Jul 13, 2024 · 6 comments · Fixed by #98898

Comments

@cadubentzen
Copy link

cadubentzen commented Jul 13, 2024

https://godbolt.org/z/KjEreWf57

With Clang 18.1 and libstdc++, we get different behavior when compared to Clang 17 in the code below.
EDIT: requires at least -O1.

#include <optional>
#include <iostream>

// defined in a separate compilation unit
int takeIntRefAndReturn0(int&);

std::optional<int> shouldReturnEmptyOptional() {
  int v = 5;
  if (takeIntRefAndReturn0(v))
    return v;
  return std::nullopt;
}

int main() {
  auto opt = shouldReturnEmptyOptional();
  if (opt && *opt > 0) {
    std::cout << "SHOULD NOT BE PRINTED: *opt = " << *opt << std::endl;
    return 1;
  }
  std::cout << "SHOULD BE PRINTED" << std::endl;
}

With Clang 17, we get SHOULD BE PRINTED, while with with Clang 18.1 we get SHOULD NOT BE PRINTED: *opt = 5.

With git-bisect, I found that this is caused by 060de41.

An isomorphic example to reproduce this (https://godbolt.org/z/9PsjY17sT):

int takeIntRefReturn0(int &);
void assertNotReached(int);

static bool logicalAnd(bool a, bool b) { return a && b; }

int main() {
  int v4;
  bool v3;
  {
    int v1 = 5;
    int v2 = takeIntRefReturn0(v1);
    v3 = v2 != 0;
    if (v3)
      v4 = v1;
  }
  // Move  to a function so that && is not short cutted.
  // v4 will be undefined if v2 == 0, but v3 is false, so the branch shouldn't be entered.
  if (logicalAnd(v3, v4 > 0))
    assertNotReached(v4);

  return 0;
}

Note in the generated LLVM IR that

%6 = icmp sgt i32 %5, 0
%7 = and i1 %3, %6
br i1 %7, label %8, label %9

was reduced to only

%6 = icmp sgt i32 %5, 0
br i1 %6, label %7, label %8
@dtcxzyw
Copy link
Member

dtcxzyw commented Jul 13, 2024

Reproducer: https://alive2.llvm.org/ce/z/p4ZGxe

; bin/opt -passes=instcombine test.ll -S
define i1 @src(i32 noundef %x, i32 %y) {
  %3 = icmp ne i32 %x, 0
  %5 = select i1 %3, i32 %y, i32 undef
  %6 = icmp sgt i32 %5, 0
  %7 = and i1 %3, %6
  br i1 %7, label %if.then, label %if.else

if.then:
  call void @use(i32 noundef %5)
  ret i1 true

if.else:
  ret i1 false
}

define i1 @tgt(i32 noundef %x, i32 %y) {
  %.not = icmp eq i32 %x, 0
  %1 = select i1 %.not, i32 undef, i32 %y
  %2 = icmp sgt i32 %1, 0
  br i1 %2, label %if.then, label %if.else

if.then:                                          ; preds = %0
  call void @use(i32 noundef %1)
  ret i1 true

if.else:                                          ; preds = %0
  ret i1 false
}

declare void @use(i32 %x)

cc @nikic

@nikic
Copy link
Contributor

nikic commented Jul 13, 2024

A bit simpler using just instsimplify: https://alive2.llvm.org/ce/z/tnkf6A

@nikic
Copy link
Contributor

nikic commented Jul 13, 2024

I expect the fix here is to perform replacements with Q.getWithoutUndef(). But probably not just here, but also in other places using simplifyWithOpReplaced(). (Possibly making it always use that mode.)

@nikic nikic self-assigned this Jul 13, 2024
nikic added a commit that referenced this issue Jul 15, 2024
nikic added a commit to nikic/llvm-project that referenced this issue Jul 15, 2024
The final case in Simplify (where Res == Absorber and the predicate
is inverted) is not generally safe when the simplification is a
refinement. In particular, we may simplify assuming a specific value
for undef, but then chose a different one later.

However, it *is* safe to refine poison in this context, unlike in
the equivalent select folds. This is the reason why this fold did
not use AllowRefinement=false in the first place, and using that
option would introduce a lot of test regressions.

This patch takes the middle path of disabling undef refinements
in particular using the getWithoutUndef() SimplifyQuery option.
However, this option doesn't actually work in this case, because
the problematic fold is inside constant folding, and we currently
don't propagate this option all the way from InstSimplify over
ConstantFolding to ConstantFold. Work around this by explicitly
checking for undef operands in simplifyWithOpReplaced().

Finally, make sure that places where AllowRefinement=false also
use Q.getWithoutUndef(). I don't have a specific test case for
this (the original one does not work because we don't simplify
selects with constant condition in this mode in the first place)
but this seems like the correct thing to do to be conservative.

Fixes llvm#98753.
nikic added a commit that referenced this issue Jul 16, 2024
)

The final case in Simplify (where Res == Absorber and the predicate is
inverted) is not generally safe when the simplification is a refinement.
In particular, we may simplify assuming a specific value for undef, but
then chose a different one later.

However, it *is* safe to refine poison in this context, unlike in the
equivalent select folds. This is the reason why this fold did not use
AllowRefinement=false in the first place, and using that option would
introduce a lot of test regressions.

This patch takes the middle path of disabling undef refinements in
particular using the getWithoutUndef() SimplifyQuery option. However,
this option doesn't actually work in this case, because the problematic
fold is inside constant folding, and we currently don't propagate this
option all the way from InstSimplify over ConstantFolding to
ConstantFold. Work around this by explicitly checking for undef operands
in simplifyWithOpReplaced().

Finally, make sure that places where AllowRefinement=false also use
Q.getWithoutUndef(). I don't have a specific test case for this (the
original one does not work because we don't simplify selects with
constant condition in this mode in the first place) but this seems like
the correct thing to do to be conservative.

Fixes #98753.
@cadubentzen
Copy link
Author

Thanks a lot for fixing this so quickly! I wonder if this should be included in 18.1.9?

@nikic
Copy link
Contributor

nikic commented Jul 16, 2024

The LLVM 18 branch is already closed, there will be no further releases.

@thesamesam
Copy link
Member

I'll add the milestone anyway as it's easier to track for downstream backports then.

@thesamesam thesamesam added this to the LLVM 18.X Release milestone Jul 20, 2024
@github-project-automation github-project-automation bot moved this to Needs Triage in LLVM Release Status Jul 20, 2024
yuxuanchen1997 pushed a commit that referenced this issue Jul 25, 2024
)

Summary:
The final case in Simplify (where Res == Absorber and the predicate is
inverted) is not generally safe when the simplification is a refinement.
In particular, we may simplify assuming a specific value for undef, but
then chose a different one later.

However, it *is* safe to refine poison in this context, unlike in the
equivalent select folds. This is the reason why this fold did not use
AllowRefinement=false in the first place, and using that option would
introduce a lot of test regressions.

This patch takes the middle path of disabling undef refinements in
particular using the getWithoutUndef() SimplifyQuery option. However,
this option doesn't actually work in this case, because the problematic
fold is inside constant folding, and we currently don't propagate this
option all the way from InstSimplify over ConstantFolding to
ConstantFold. Work around this by explicitly checking for undef operands
in simplifyWithOpReplaced().

Finally, make sure that places where AllowRefinement=false also use
Q.getWithoutUndef(). I don't have a specific test case for this (the
original one does not work because we don't simplify selects with
constant condition in this mode in the first place) but this seems like
the correct thing to do to be conservative.

Fixes #98753.

Test Plan: 

Reviewers: 

Subscribers: 

Tasks: 

Tags: 


Differential Revision: https://phabricator.intern.facebook.com/D60251555
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Needs Triage
Development

Successfully merging a pull request may close this issue.

5 participants