[PATCH] D154359: [clang] Reset FP options before template instantiation

[Hubert Tong via Phabricator via cfe-commits]

hubert.reinterpretcast added a comment.

This patch seems to be direct cause of a regression: 
https://github.com/llvm/llvm-project/issues/64605.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154359/new/

https://reviews.llvm.org/D154359

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154359: [clang] Reset FP options before template instantiation

[Serge Pavlov via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGfde5924dcc69: [clang] Reset FP options before template 
instantiation (authored by sepavloff).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154359/new/

https://reviews.llvm.org/D154359

Files:
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/CodeGen/fp-template.cpp


Index: clang/test/CodeGen/fp-template.cpp
===
--- clang/test/CodeGen/fp-template.cpp
+++ clang/test/CodeGen/fp-template.cpp
@@ -45,10 +45,24 @@
 // CHECK:   call float @llvm.experimental.constrained.fsub.f32({{.*}}, 
metadata !"round.upward", metadata !"fpexcept.ignore")
 
 
-// This pragma sets non-default rounding mode before delayed parsing occurs. It
-// is used to check that the parsing uses FP options defined by command line
-// options or by pragma before the template definition but not by this pragma.
-#pragma STDC FENV_ROUND FE_TOWARDZERO
+#pragma STDC FENV_ROUND FE_TONEAREST
+
+namespace PR63542 {
+  template  float stable_sort(float x, Compare) {
+float result = x + x;
+stable_sort(x, int());
+return result;
+  }
+  float linkage_wrap() { return stable_sort(0.0, 1); }
+}
 
+// CHECK-LABEL: define {{.*}} float @_ZN7PR6354211stable_sortIiEEffT_(
+// CHECK: fadd float
+
+// These pragmas set non-default FP environment before delayed parsing occurs.
+// It is used to check that the parsing uses FP options defined by command line
+// options or by pragma before the template definition but not by these 
pragmas.
+#pragma STDC FENV_ROUND FE_TOWARDZERO
+#pragma STDC FENV_ACCESS ON
 
 // CHECK: attributes #[[ATTR01]] = { {{.*}}strictfp
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -5087,6 +5087,10 @@
 // PushDeclContext because we don't have a scope.
 Sema::ContextRAII savedContext(*this, Function);
 
+FPFeaturesStateRAII SavedFPFeatures(*this);
+CurFPFeatures = FPOptions(getLangOpts());
+FpPragmaStack.CurrentValue = FPOptionsOverride();
+
 if (addInstantiatedParametersToScope(Function, PatternDecl, Scope,
  TemplateArgs))
   return;


Index: clang/test/CodeGen/fp-template.cpp
===
--- clang/test/CodeGen/fp-template.cpp
+++ clang/test/CodeGen/fp-template.cpp
@@ -45,10 +45,24 @@
 // CHECK:   call float @llvm.experimental.constrained.fsub.f32({{.*}}, metadata !"round.upward", metadata !"fpexcept.ignore")
 
 
-// This pragma sets non-default rounding mode before delayed parsing occurs. It
-// is used to check that the parsing uses FP options defined by command line
-// options or by pragma before the template definition but not by this pragma.
-#pragma STDC FENV_ROUND FE_TOWARDZERO
+#pragma STDC FENV_ROUND FE_TONEAREST
+
+namespace PR63542 {
+  template  float stable_sort(float x, Compare) {
+float result = x + x;
+stable_sort(x, int());
+return result;
+  }
+  float linkage_wrap() { return stable_sort(0.0, 1); }
+}
 
+// CHECK-LABEL: define {{.*}} float @_ZN7PR6354211stable_sortIiEEffT_(
+// CHECK: fadd float
+
+// These pragmas set non-default FP environment before delayed parsing occurs.
+// It is used to check that the parsing uses FP options defined by command line
+// options or by pragma before the template definition but not by these pragmas.
+#pragma STDC FENV_ROUND FE_TOWARDZERO
+#pragma STDC FENV_ACCESS ON
 
 // CHECK: attributes #[[ATTR01]] = { {{.*}}strictfp
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -5087,6 +5087,10 @@
 // PushDeclContext because we don't have a scope.
 Sema::ContextRAII savedContext(*this, Function);
 
+FPFeaturesStateRAII SavedFPFeatures(*this);
+CurFPFeatures = FPOptions(getLangOpts());
+FpPragmaStack.CurrentValue = FPOptionsOverride();
+
 if (addInstantiatedParametersToScope(Function, PatternDecl, Scope,
  TemplateArgs))
   return;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154359: [clang] Reset FP options before template instantiation

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added a comment.

LGTM. Thanks for the patch.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154359/new/

https://reviews.llvm.org/D154359

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154359: [clang] Reset FP options before template instantiation

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff added inline comments.



Comment at: clang/test/CodeGen/fp-template.cpp:28
+// CHECK-LABEL: define {{.*}} float @_ZN7PR6354211stable_sortIiEEffT_(
+// CHECK: fadd float
+

zahiraam wrote:
> Shouldn't we be expecting a constraint add here?
Yes, you are right. I incorrectly merged the fix to main branch.

Thank you!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154359/new/

https://reviews.llvm.org/D154359

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154359: [clang] Reset FP options before template instantiation

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff updated this revision to Diff 539597.
sepavloff added a comment.

Fix test


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154359/new/

https://reviews.llvm.org/D154359

Files:
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/CodeGen/fp-template.cpp


Index: clang/test/CodeGen/fp-template.cpp
===
--- clang/test/CodeGen/fp-template.cpp
+++ clang/test/CodeGen/fp-template.cpp
@@ -45,10 +45,24 @@
 // CHECK:   call float @llvm.experimental.constrained.fsub.f32({{.*}}, 
metadata !"round.upward", metadata !"fpexcept.ignore")
 
 
-// This pragma sets non-default rounding mode before delayed parsing occurs. It
-// is used to check that the parsing uses FP options defined by command line
-// options or by pragma before the template definition but not by this pragma.
-#pragma STDC FENV_ROUND FE_TOWARDZERO
+#pragma STDC FENV_ROUND FE_TONEAREST
+
+namespace PR63542 {
+  template  float stable_sort(float x, Compare) {
+float result = x + x;
+stable_sort(x, int());
+return result;
+  }
+  float linkage_wrap() { return stable_sort(0.0, 1); }
+}
 
+// CHECK-LABEL: define {{.*}} float @_ZN7PR6354211stable_sortIiEEffT_(
+// CHECK: fadd float
+
+// These pragmas set non-default FP environment before delayed parsing occurs.
+// It is used to check that the parsing uses FP options defined by command line
+// options or by pragma before the template definition but not by these 
pragmas.
+#pragma STDC FENV_ROUND FE_TOWARDZERO
+#pragma STDC FENV_ACCESS ON
 
 // CHECK: attributes #[[ATTR01]] = { {{.*}}strictfp
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -5087,6 +5087,10 @@
 // PushDeclContext because we don't have a scope.
 Sema::ContextRAII savedContext(*this, Function);
 
+FPFeaturesStateRAII SavedFPFeatures(*this);
+CurFPFeatures = FPOptions(getLangOpts());
+FpPragmaStack.CurrentValue = FPOptionsOverride();
+
 if (addInstantiatedParametersToScope(Function, PatternDecl, Scope,
  TemplateArgs))
   return;


Index: clang/test/CodeGen/fp-template.cpp
===
--- clang/test/CodeGen/fp-template.cpp
+++ clang/test/CodeGen/fp-template.cpp
@@ -45,10 +45,24 @@
 // CHECK:   call float @llvm.experimental.constrained.fsub.f32({{.*}}, metadata !"round.upward", metadata !"fpexcept.ignore")
 
 
-// This pragma sets non-default rounding mode before delayed parsing occurs. It
-// is used to check that the parsing uses FP options defined by command line
-// options or by pragma before the template definition but not by this pragma.
-#pragma STDC FENV_ROUND FE_TOWARDZERO
+#pragma STDC FENV_ROUND FE_TONEAREST
+
+namespace PR63542 {
+  template  float stable_sort(float x, Compare) {
+float result = x + x;
+stable_sort(x, int());
+return result;
+  }
+  float linkage_wrap() { return stable_sort(0.0, 1); }
+}
 
+// CHECK-LABEL: define {{.*}} float @_ZN7PR6354211stable_sortIiEEffT_(
+// CHECK: fadd float
+
+// These pragmas set non-default FP environment before delayed parsing occurs.
+// It is used to check that the parsing uses FP options defined by command line
+// options or by pragma before the template definition but not by these pragmas.
+#pragma STDC FENV_ROUND FE_TOWARDZERO
+#pragma STDC FENV_ACCESS ON
 
 // CHECK: attributes #[[ATTR01]] = { {{.*}}strictfp
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -5087,6 +5087,10 @@
 // PushDeclContext because we don't have a scope.
 Sema::ContextRAII savedContext(*this, Function);
 
+FPFeaturesStateRAII SavedFPFeatures(*this);
+CurFPFeatures = FPOptions(getLangOpts());
+FpPragmaStack.CurrentValue = FPOptionsOverride();
+
 if (addInstantiatedParametersToScope(Function, PatternDecl, Scope,
  TemplateArgs))
   return;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154359: [clang] Reset FP options before template instantiation

[Zahira Ammarguellat via Phabricator via cfe-commits]

zahiraam added inline comments.



Comment at: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp:5093
+FpPragmaStack.CurrentValue = FPOptionsOverride();
+
 if (addInstantiatedParametersToScope(Function, PatternDecl, Scope,

This seems to be fixing the crash. 



Comment at: clang/test/CodeGen/fp-template.cpp:28
+// CHECK-LABEL: define {{.*}} float @_ZN7PR6354211stable_sortIiEEffT_(
+// CHECK: fadd float
+

Shouldn't we be expecting a constraint add here?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154359/new/

https://reviews.llvm.org/D154359

___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D154359: [clang] Reset FP options before template instantiation

[Serge Pavlov via Phabricator via cfe-commits]

sepavloff created this revision.
sepavloff added reviewers: rsmith, rjmccall, aaron.ballman, efriedma.
Herald added a project: All.
sepavloff requested review of this revision.
Herald added a project: clang.

AST nodes that may depend on FP options keep them as a difference
relative to the options outside the AST node. At the moment of
instantiation the FP options may be different from the default values,
defined by command-line option. In such case FP attributes would have
unexpected values. For example, the code:

  template  void func_01(int last, C) {
func_01(last, int());
  }
  void func_02() { func_01(0, 1); }
  #pragma STDC FENV_ACCESS ON

caused compiler crash, because template instantiation takes place at the
end of translation unit, where pragma STDC FENV_ACCESS is in effect. As
a result, code in the template instantiation would use constrained
intrinsics while the function does not have StrictFP attribute.

To solve this problem, FP attributes in Sema must be set to default
values, defined by command line options.

This change resolves https://github.com/llvm/llvm-project/issues/63542.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D154359

Files:
  clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
  clang/test/CodeGen/fp-template.cpp


Index: clang/test/CodeGen/fp-template.cpp
===
--- clang/test/CodeGen/fp-template.cpp
+++ clang/test/CodeGen/fp-template.cpp
@@ -15,4 +15,19 @@
 // CHECK-SAME:  (float noundef %{{.*}}, float noundef %{{.*}}) 
#[[ATTR01:[0-9]+]]{{.*}} {
 // CHECK:   call float @llvm.experimental.constrained.fadd.f32
 
+namespace PR63542 {
+  template  float stable_sort(float x, Compare) {
+float result = x + x;
+stable_sort(x, int());
+return result;
+  }
+  float linkage_wrap() { return stable_sort(0.0, 1); }
+}
+
+// CHECK-LABEL: define {{.*}} float @_ZN7PR6354211stable_sortIiEEffT_(
+// CHECK: fadd float
+
+// Must be at the end of translation unit.
+#pragma STDC FENV_ACCESS ON
+
 // CHECK: attributes #[[ATTR01]] = { {{.*}}strictfp
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -5087,6 +5087,10 @@
 // PushDeclContext because we don't have a scope.
 Sema::ContextRAII savedContext(*this, Function);
 
+FPFeaturesStateRAII SavedFPFeatures(*this);
+CurFPFeatures = FPOptions(getLangOpts());
+FpPragmaStack.CurrentValue = FPOptionsOverride();
+
 if (addInstantiatedParametersToScope(Function, PatternDecl, Scope,
  TemplateArgs))
   return;


Index: clang/test/CodeGen/fp-template.cpp
===
--- clang/test/CodeGen/fp-template.cpp
+++ clang/test/CodeGen/fp-template.cpp
@@ -15,4 +15,19 @@
 // CHECK-SAME:  (float noundef %{{.*}}, float noundef %{{.*}}) #[[ATTR01:[0-9]+]]{{.*}} {
 // CHECK:   call float @llvm.experimental.constrained.fadd.f32
 
+namespace PR63542 {
+  template  float stable_sort(float x, Compare) {
+float result = x + x;
+stable_sort(x, int());
+return result;
+  }
+  float linkage_wrap() { return stable_sort(0.0, 1); }
+}
+
+// CHECK-LABEL: define {{.*}} float @_ZN7PR6354211stable_sortIiEEffT_(
+// CHECK: fadd float
+
+// Must be at the end of translation unit.
+#pragma STDC FENV_ACCESS ON
+
 // CHECK: attributes #[[ATTR01]] = { {{.*}}strictfp
Index: clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
===
--- clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
+++ clang/lib/Sema/SemaTemplateInstantiateDecl.cpp
@@ -5087,6 +5087,10 @@
 // PushDeclContext because we don't have a scope.
 Sema::ContextRAII savedContext(*this, Function);
 
+FPFeaturesStateRAII SavedFPFeatures(*this);
+CurFPFeatures = FPOptions(getLangOpts());
+FpPragmaStack.CurrentValue = FPOptionsOverride();
+
 if (addInstantiatedParametersToScope(Function, PatternDecl, Scope,
  TemplateArgs))
   return;
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits