[Regression 16 -> 17] Template instantiation ignores FENV_ACCESS being ON for both definition and instantiation #64605

hubert-reinterpretcast · 2023-08-10T22:34:34Z

Consider:

#pragma STDC FENV_ACCESS ON
template <typename>
int b() {
  int x;
  if ((float)0xFFFFFFFF != (float)0x100000000) {
    x = 1;
  }
  return x;
}
int f() { return b<void>(); }

Clang 16 generates the floating-point operations (as is appropriate for FENV_ACCESS being ON):

f():                                  # @f()
        mov     eax, 4294967295
        cvtsi2ss        xmm0, rax
        movabs  rax, 4294967296
        cvtsi2ss        xmm1, rax
        ucomiss xmm0, xmm1
        mov     eax, 1
        ret

Clang "17" currently generates no floating-point operations:

f(): # @f()
  ret

Compiler Explorer: https://godbolt.org/z/PcoaaxzKn

Reverting fde5924 restores the previous behaviour.

The text was updated successfully, but these errors were encountered:

llvmbot · 2023-08-10T22:34:48Z

@llvm/issue-subscribers-clang-frontend

spavloff · 2023-08-12T09:39:00Z

This defect is not caused by fde5924. The reason is the elimination of ImplicitCastExpr when a template is instantiated, see comment:

llvm-project/clang/lib/Sema/TreeTransform.h

Lines 11629 to 11631 in a5fe6c7

    
           TreeTransform<Derived>::TransformImplicitCastExpr(ImplicitCastExpr *E) { 
        
             // Implicit casts are eliminated during transformation, since they 
        
             // will be recomputed by semantic analysis after transformation.

ImplicitCastExpr keeps FP options, including rounding direction. Removing them results in FPOptions loss, they are taken from the current Sema state, which is incorrect behavior.

Revert of fde5924 accidentally fixed the behavior, because FP options used to generate code were taken from Sema state (and nof from AST node) and they were the same as at the point of the template definition. If, for example, #pragma STDC FENV_ACCESS OFF is put at the end of the file, the problem would be observed with the commit reverted.

I will prepare a fix soon.

AaronBallman · 2023-08-16T16:03:22Z

FWIW, rc3 is supposed to go out in about a week and I believe we're hoping that's one of the last rcs, so we don't have much bake time for a fix. If the fix ends up being trivial and obviously correct, then I think it may still be able to make 17.x. I'm a bit more worried about it not being a trivial change though. It sounds like fde5924 accidentally exposed an issue that is causing more miscompiles and a revert of it would get us back to the same stability as Clang 16.x, even though it's not actually a "fix" per se. Am I correct? If so, we may want to consider doing that revert from the 17.x branch so we don't introduce more miscompilations and then do the real fix for 18.x with plenty of time for testing fallout.

spavloff · 2023-08-17T09:09:43Z

I put a patch https://reviews.llvm.org/D158158, which should fix the issue.It is not a full-fledged solution (it does not support variable templates), but it should be enough for this issue. It is small and obvious, hopefully this can facilitate the review.

fde5924 fixes #63542, so revert of it exposes that issue.

AaronBallman · 2023-08-17T11:28:46Z

fde5924 fixes #63542, so revert of it exposes that issue.

To make sure I'm on the same page -- a revert would bring us back to the Clang 16 behavior (bugs and all)?

hubert-reinterpretcast · 2023-08-19T01:40:47Z

It is not a full-fledged solution (it does not support variable templates), but it should be enough for this issue.

It seems variable templates have another layer of problems to begin with.

Clang 16 will not generate a call to foo for the following:

#include <float.h>
struct ChangeRounding {
  ChangeRounding();
  ~ChangeRounding();
};
int foo(), bar();

#pragma STDC FENV_ACCESS ON

template <typename T>
int var = (ChangeRounding(), (T)0xFFFFFFFF != (T)0x100000000 ? foo() : bar());

int main() {
  return var<float>;
}

https://godbolt.org/z/Mnq5PoP8o

AaronBallman · 2023-08-21T11:43:08Z

Those changes do seem quite simple and I think they're safe for a backport to 17.x, but I'd appreciate a few opinions from @hubert-reinterpretcast and @jcranmer-intel to see if they agree we should backport. The alternatives are: leave the regression in Clang 16, or revert the original changes from just the 17.x branch so we're back in the (previously also bad) state we were in for 16.

spavloff · 2023-08-21T16:15:19Z

The change 0baf85c requires also 73e5a70. The latter fixes a test from the former.

hubert-reinterpretcast · 2023-08-21T17:12:20Z

Those changes do seem quite simple and I think they're safe for a backport to 17.x

I agree, and I think we should backport. The original changes fixed an issue in Clang 16 that was encountered by multiple parties (so reverting is not my first choice). With the additional changes, the "regressions" known to me are addressed.

AaronBallman · 2023-08-21T18:15:32Z

Excellent, thank you @hubert-reinterpretcast

@spavloff, can you handle the backport for both commits to 17.x? (I think it's fine to squash them into one commit and only put up one PR)

When an expression is instantiated, TreeTransform skips ImplicitCastExpr nodes, assuming they are recreated when the instantiated expression is built. It breaks functions that use non-default floating-point options, because they are kept in these ImplicitCastExprs. In this case the recreated ImplicitCastExpr takes FP options from the current Sema state and not from AST node. To fix this issue the FP options in Sema object are set when a compound statement is cloned in TreeTransform. This change fixes llvm#64605 ([Regression 16 -> 17] Template instantiation ignores FENV_ACCESS being ON for both definition and instantiation). Differential Revision: https://reviews.llvm.org/D158158 (cherry picked from commit 0baf85c) (cherry picked from commit 73e5a70)

spavloff · 2023-08-22T06:47:55Z

/cherry-pick 0baf85c
/cherry-pick 73e5a70

llvmbot · 2023-08-22T06:55:55Z

/branch llvm/llvm-project-release-prs/issue64605

When an expression is instantiated, TreeTransform skips ImplicitCastExpr nodes, assuming they are recreated when the instantiated expression is built. It breaks functions that use non-default floating-point options, because they are kept in these ImplicitCastExprs. In this case the recreated ImplicitCastExpr takes FP options from the current Sema state and not from AST node. To fix this issue the FP options in Sema object are set when a compound statement is cloned in TreeTransform. This change fixes llvm/llvm-project#64605 ([Regression 16 -> 17] Template instantiation ignores FENV_ACCESS being ON for both definition and instantiation). Differential Revision: https://reviews.llvm.org/D158158 (cherry picked from commit 0baf85c331090fbe2d2b42214ed0664d55feb0b5)

llvmbot · 2023-08-22T07:03:48Z

/pull-request llvm/llvm-project-release-prs#627

spavloff · 2023-08-22T13:35:52Z

/cherry-pick 0baf85c 73e5a70

dyung · 2023-08-27T07:40:47Z

Not sure where to put this, but ever since 73e5a70 was cherry-picked to the release/17.x branch as 73e5a70, the test clang/test/SemaCXX/template-64605.cpp has been failing on multiple release bots:

tru · 2023-08-27T08:51:40Z

Not sure where to put this, but ever since 73e5a70 was cherry-picked to the release/17.x branch as 73e5a70, the test clang/test/SemaCXX/template-64605.cpp has been failing on multiple release bots:

https://lab.llvm.org/buildbot/#/builders/148/builds/354

https://lab.llvm.org/buildbot/#/builders/143/builds/524

https://lab.llvm.org/buildbot/#/builders/144/builds/505

https://lab.llvm.org/buildbot/#/builders/260/builds/35

https://lab.llvm.org/buildbot/#/builders/261/builds/35

~~This should be fixed, I just forgot to push the commit. Will do that now.~~

Never mind, I thought about another issue. Yes this seems like a problem. @spavloff can you have a look?

tru · 2023-08-28T10:14:31Z

I ran the tests locally now and I needed to revert both e54f483 and f1d5ea3 in order for the tests to pass again. I have pushed the revert commits to the release branch for now. I will re-open this issue and hope that @spavloff can have a look at fixing the tests and re-cherry-pick the fix.

tru · 2023-08-28T10:15:30Z

0638df0
baae3c3

jonathonpenix · 2023-08-28T18:18:56Z

Hi, apologies in advance if this has already been addressed/mentioned elsewhere/is obvious, but in case it is helpful: I ran into this downstream and I think the failing test might just need the "implicit_instantiation" bit removed (or another commit brought in). IIUC, the ability to output this was added in 3bfafc4 which is present in main but not the release_17.x branch--template-64605.cpp was added after 3bfafc4 in main, so it expectedly has the "implicit_instantation" bit, but we don't know how to generate this in the release branch.

Again though, apologies in advance if I'm misunderstanding something/this has already been handled!

AaronBallman · 2023-08-28T18:44:47Z

Again though, apologies in advance if I'm misunderstanding something/this has already been handled!

I think you nailed the problem!

spavloff · 2023-08-30T05:57:15Z

@jpenix-quic Thank you for your analysis!

A check pattern in clang/test/SemaCXX/template-64605.cpp contains template specialization kind (the text "implicit_instantiation"). It does not need to be checked and can be safely removed. Presence of this text in the check pattern prevents from backporting some commits to the release branch: #64605. It has only recently been printed and the relevant commit is not present in the release/17.x branch.

spavloff · 2023-08-30T07:41:32Z

/cherry-pick 0baf85c 73e5a70 8859c64

llvmbot · 2023-08-30T07:49:00Z

/branch llvm/llvm-project-release-prs/issue64605

When an expression is instantiated, TreeTransform skips ImplicitCastExpr nodes, assuming they are recreated when the instantiated expression is built. It breaks functions that use non-default floating-point options, because they are kept in these ImplicitCastExprs. In this case the recreated ImplicitCastExpr takes FP options from the current Sema state and not from AST node. To fix this issue the FP options in Sema object are set when a compound statement is cloned in TreeTransform. This change fixes llvm/llvm-project#64605 ([Regression 16 -> 17] Template instantiation ignores FENV_ACCESS being ON for both definition and instantiation). Differential Revision: https://reviews.llvm.org/D158158 (cherry picked from commit 0baf85c331090fbe2d2b42214ed0664d55feb0b5)

A check pattern in clang/test/SemaCXX/template-64605.cpp contains template specialization kind (the text "implicit_instantiation"). It does not need to be checked and can be safely removed. Presence of this text in the check pattern prevents from backporting some commits to the release branch: llvm/llvm-project#64605. It has only recently been printed and the relevant commit is not present in the release/17.x branch. (cherry picked from commit 8859c644ede4898f90f77dcad2286de08a9ba62e)

llvmbot · 2023-08-30T07:56:56Z

/pull-request llvm/llvm-project-release-prs#666

When an expression is instantiated, TreeTransform skips ImplicitCastExpr nodes, assuming they are recreated when the instantiated expression is built. It breaks functions that use non-default floating-point options, because they are kept in these ImplicitCastExprs. In this case the recreated ImplicitCastExpr takes FP options from the current Sema state and not from AST node. To fix this issue the FP options in Sema object are set when a compound statement is cloned in TreeTransform. This change fixes llvm/llvm-project#64605 ([Regression 16 -> 17] Template instantiation ignores FENV_ACCESS being ON for both definition and instantiation). Differential Revision: https://reviews.llvm.org/D158158 (cherry picked from commit 0baf85c331090fbe2d2b42214ed0664d55feb0b5)

A check pattern in clang/test/SemaCXX/template-64605.cpp contains template specialization kind (the text "implicit_instantiation"). It does not need to be checked and can be safely removed. Presence of this text in the check pattern prevents from backporting some commits to the release branch: llvm/llvm-project#64605. It has only recently been printed and the relevant commit is not present in the release/17.x branch. (cherry picked from commit 8859c644ede4898f90f77dcad2286de08a9ba62e)

When an expression is instantiated, TreeTransform skips ImplicitCastExpr nodes, assuming they are recreated when the instantiated expression is built. It breaks functions that use non-default floating-point options, because they are kept in these ImplicitCastExprs. In this case the recreated ImplicitCastExpr takes FP options from the current Sema state and not from AST node. To fix this issue the FP options in Sema object are set when a compound statement is cloned in TreeTransform. This change fixes llvm#64605 ([Regression 16 -> 17] Template instantiation ignores FENV_ACCESS being ON for both definition and instantiation). Differential Revision: https://reviews.llvm.org/D158158

hubert-reinterpretcast added clang:frontend Language frontend issues, e.g. anything involving "Sema" regression miscompilation floating-point Floating-point math labels Aug 10, 2023

hubert-reinterpretcast added this to the LLVM 17.0.X Release milestone Aug 10, 2023

github-project-automation bot added this to LLVM Release Status Aug 10, 2023

github-project-automation bot moved this to Needs Triage in LLVM Release Status Aug 10, 2023

hubert-reinterpretcast assigned spavloff Aug 10, 2023

spavloff mentioned this issue Aug 12, 2023

[clang code-gen] #pragma FENV_ACCESS leaking across template declarations/definitions #63063

Closed

spavloff closed this as completed in 0baf85c Aug 21, 2023

tru moved this from Needs Triage to Done in LLVM Release Status Aug 21, 2023

spavloff mentioned this issue Aug 22, 2023

PR for llvm/llvm-project#64605 #64889

Closed

llvmbot mentioned this issue Aug 22, 2023

PR for llvm/llvm-project#64605 llvm/llvm-project-release-prs#627

Merged

tru moved this from Needs Merge to Done in LLVM Release Status Aug 25, 2023

EugeneZelenko added the release:merged label Aug 25, 2023

tru reopened this Aug 28, 2023

tru moved this from Done to Needs Pull Request in LLVM Release Status Aug 28, 2023

llvmbot mentioned this issue Aug 30, 2023

PR for llvm/llvm-project#64605 llvm/llvm-project-release-prs#666

Merged

tru moved this from Needs Pull Request to Needs Review in LLVM Release Status Aug 30, 2023

tru closed this as completed in llvm/llvm-project-release-prs#666 Aug 30, 2023

tru moved this from Needs Review to Done in LLVM Release Status Aug 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Regression 16 -> 17] Template instantiation ignores FENV_ACCESS being ON for both definition and instantiation #64605

[Regression 16 -> 17] Template instantiation ignores FENV_ACCESS being ON for both definition and instantiation #64605

hubert-reinterpretcast commented Aug 10, 2023

llvmbot commented Aug 10, 2023

spavloff commented Aug 12, 2023

AaronBallman commented Aug 16, 2023

spavloff commented Aug 17, 2023

AaronBallman commented Aug 17, 2023

hubert-reinterpretcast commented Aug 19, 2023

AaronBallman commented Aug 21, 2023

spavloff commented Aug 21, 2023

hubert-reinterpretcast commented Aug 21, 2023

AaronBallman commented Aug 21, 2023

spavloff commented Aug 22, 2023

llvmbot commented Aug 22, 2023

llvmbot commented Aug 22, 2023

spavloff commented Aug 22, 2023

dyung commented Aug 27, 2023

tru commented Aug 27, 2023 •

edited

Loading

tru commented Aug 28, 2023

tru commented Aug 28, 2023

jonathonpenix commented Aug 28, 2023 •

edited

Loading

AaronBallman commented Aug 28, 2023

spavloff commented Aug 30, 2023

spavloff commented Aug 30, 2023

llvmbot commented Aug 30, 2023

llvmbot commented Aug 30, 2023

[Regression 16 -> 17] Template instantiation ignores FENV_ACCESS being ON for both definition and instantiation #64605

[Regression 16 -> 17] Template instantiation ignores FENV_ACCESS being ON for both definition and instantiation #64605

Comments

hubert-reinterpretcast commented Aug 10, 2023

llvmbot commented Aug 10, 2023

spavloff commented Aug 12, 2023

AaronBallman commented Aug 16, 2023

spavloff commented Aug 17, 2023

AaronBallman commented Aug 17, 2023

hubert-reinterpretcast commented Aug 19, 2023

AaronBallman commented Aug 21, 2023

spavloff commented Aug 21, 2023

hubert-reinterpretcast commented Aug 21, 2023

AaronBallman commented Aug 21, 2023

spavloff commented Aug 22, 2023

llvmbot commented Aug 22, 2023

llvmbot commented Aug 22, 2023

spavloff commented Aug 22, 2023

dyung commented Aug 27, 2023

tru commented Aug 27, 2023 • edited Loading

tru commented Aug 28, 2023

tru commented Aug 28, 2023

jonathonpenix commented Aug 28, 2023 • edited Loading

AaronBallman commented Aug 28, 2023

spavloff commented Aug 30, 2023

spavloff commented Aug 30, 2023

llvmbot commented Aug 30, 2023

llvmbot commented Aug 30, 2023

tru commented Aug 27, 2023 •

edited

Loading

jonathonpenix commented Aug 28, 2023 •

edited

Loading