Skip to content

Wrong code at -Os on x86-64_gnu-linux (recent regression) #82243

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
shao-hua-li opened this issue Feb 19, 2024 · 3 comments · Fixed by #82352
Closed

Wrong code at -Os on x86-64_gnu-linux (recent regression) #82243

shao-hua-li opened this issue Feb 19, 2024 · 3 comments · Fixed by #82352
Assignees
Labels

Comments

@shao-hua-li
Copy link

Clang at -Os produced the wrong code.

Bisected to dce77a3, which was committed by @fhahn

Compiler explorer: https://godbolt.org/z/fTrenPK4h

% cat reduced.c
int printf(const char *, ...);
int a, b, c, d, e;
int f[9];
int g(int i) {
  c = 1 << i;
  if (b & c)
    return 4;
  return 0;
}
int j(int i) {
  int h = g(i);
  return h;
}
int k() {
  d = 6;
  for (; d; d--) {
    e = 0;
    for (; e <= 6; e++) {
      f[j(d - 1)] = f[d];
      for (; e + d;)
        return 0;
    }
  }
  return 0;
}
int main() {
  k();
  printf("%d\n", a);
}
%
% clang -O3 reduced.c && ./a.out
0
% clang -Os reduced.c && ./a.out

%
@fhahn
Copy link
Contributor

fhahn commented Feb 19, 2024

The issue is that IndVars doesn't drop poison-generating flags when hoisting the IV to introduce a new use of the widened IV. working on a fix

fhahn added a commit that referenced this issue Feb 20, 2024
fhahn added a commit to fhahn/llvm-project that referenced this issue Feb 20, 2024
widenIVUse may hoist a wide induction increment and introduce new uses,
but does not recompute the wrap flags. In some cases this can make the
new uses of the wide IV inc more poisonous.

Update the code to recompute flags if needed when hoisting an IV. If
both the narrow and wide IV increment's flags match and we can re-use
the flags from the increments, there's no need to recompute the flags,
as the replacement won't make the new uses of the wide IV's increment
more poisonous.

Note that this also updates a stale comment which claimed that the widen
increment is only used if it dominates the new use.

Happy to move out introducing canReuseFlagsFromOriginalIVInc to a
separate patch if desired and once we agree on the API.

The helper should also be used to guard the code added in da43733,
which I am planning on doing separately once the helper lands.

Fixes llvm#82243.
fhahn added a commit that referenced this issue Feb 20, 2024
…82352)

widenIVUse may hoist a wide induction increment and introduce new uses,
but does not recompute the wrap flags. In some cases this can make the
new uses of the wide IV inc more poisonous.

Update the code to recompute flags if needed when hoisting an IV. If
both the narrow and wide IV increment's flags match and we can re-use
the flags from the increments, there's no need to recompute the flags,
as the replacement won't make the new uses of the wide IV's increment
more poisonous.

Note that this also updates a stale comment which claimed that the widen
increment is only used if it dominates the new use.

The helper should also be used to guard the code added in da43733,
which I am planning on doing separately once the helper lands.

Fixes #82243.
@EugeneZelenko EugeneZelenko added llvm:SCEV Scalar Evolution and removed llvm:transforms labels Feb 20, 2024
@fhahn
Copy link
Contributor

fhahn commented Feb 20, 2024

Should be fixed, thanks again as always @shao-hua-li

@shao-hua-li
Copy link
Author

You're welcome @fhahn

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants