[PATCH] D102995: errorUnsupported should be non-fatal

[MJ via Phabricator via cfe-commits]

majiang31312 updated this revision to Diff 347545.
majiang31312 set the repository for this revision to rG LLVM Github Monorepo.
majiang31312 added a comment.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

CI failed to patch?   Retry with Repository 'rG 
 LLVM Github Monorepo'


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102995

Files:
  clang/test/CodeGen/x86_64-mno-sse.c
  clang/test/CodeGen/x86_64-mno-sse2.c
  llvm/lib/Target/X86/X86ISelLowering.cpp


Index: llvm/lib/Target/X86/X86ISelLowering.cpp
===
--- llvm/lib/Target/X86/X86ISelLowering.cpp
+++ llvm/lib/Target/X86/X86ISelLowering.cpp
@@ -96,8 +96,8 @@
 static void errorUnsupported(SelectionDAG &DAG, const SDLoc &dl,
  const char *Msg) {
   MachineFunction &MF = DAG.getMachineFunction();
-  DAG.getContext()->diagnose(
-  DiagnosticInfoUnsupported(MF.getFunction(), Msg, dl.getDebugLoc()));
+  DAG.getContext()->diagnose(DiagnosticInfoUnsupported(
+  MF.getFunction(), Msg, dl.getDebugLoc(), DS_Warning));
 }
 
 X86TargetLowering::X86TargetLowering(const X86TargetMachine &TM,
Index: clang/test/CodeGen/x86_64-mno-sse2.c
===
--- clang/test/CodeGen/x86_64-mno-sse2.c
+++ clang/test/CodeGen/x86_64-mno-sse2.c
@@ -1,11 +1,11 @@
 // RUN: %clang_cc1 -triple x86_64-linux -target-feature -sse2 -S -o /dev/null 
-verify %s
 // REQUIRES: x86-registered-target
 
-double f1(void) { // expected-error {{SSE2 register return with SSE2 disabled}}
+double f1(void) { // expected-warning {{SSE2 register return with SSE2 
disabled}}
   return 1.4;
 }
 extern double g;
-void f2(void) { // expected-error {{SSE2 register return with SSE2 disabled}}
+void f2(void) { // expected-warning {{SSE2 register return with SSE2 disabled}}
   g = f1();
 }
 void take_double(double);
@@ -15,6 +15,6 @@
 }
 
 double return_double();
-void call_double(double *a) { // expected-error {{SSE2 register return with 
SSE2 disabled}}
+void call_double(double *a) { // expected-warning {{SSE2 register return with 
SSE2 disabled}}
   *a = return_double();
 }
Index: clang/test/CodeGen/x86_64-mno-sse.c
===
--- clang/test/CodeGen/x86_64-mno-sse.c
+++ clang/test/CodeGen/x86_64-mno-sse.c
@@ -1,15 +1,14 @@
 // RUN: %clang_cc1 -triple x86_64-linux -target-feature -sse -target-feature 
-sse2 -S -o /dev/null -verify %s
 // REQUIRES: x86-registered-target
 
-double f1(void) { // expected-error {{SSE register return with SSE disabled}}
+double f1(void) { // expected-warning {{SSE register return with SSE disabled}}
   return 1.4;
 }
 extern double g;
-void f2(void) { // expected-error {{SSE register return with SSE disabled}}
+void f2(void) { // expected-warning {{SSE register return with SSE disabled}}
   g = f1();
 }
 void take_double(double);
 void pass_double(void) {
-  // FIXME: Still asserts.
-  //take_double(1.5);
+  take_double(1.5);
 }


Index: llvm/lib/Target/X86/X86ISelLowering.cpp
===
--- llvm/lib/Target/X86/X86ISelLowering.cpp
+++ llvm/lib/Target/X86/X86ISelLowering.cpp
@@ -96,8 +96,8 @@
 static void errorUnsupported(SelectionDAG &DAG, const SDLoc &dl,
  const char *Msg) {
   MachineFunction &MF = DAG.getMachineFunction();
-  DAG.getContext()->diagnose(
-  DiagnosticInfoUnsupported(MF.getFunction(), Msg, dl.getDebugLoc()));
+  DAG.getContext()->diagnose(DiagnosticInfoUnsupported(
+  MF.getFunction(), Msg, dl.getDebugLoc(), DS_Warning));
 }
 
 X86TargetLowering::X86TargetLowering(const X86TargetMachine &TM,
Index: clang/test/CodeGen/x86_64-mno-sse2.c
===
--- clang/test/CodeGen/x86_64-mno-sse2.c
+++ clang/test/CodeGen/x86_64-mno-sse2.c
@@ -1,11 +1,11 @@
 // RUN: %clang_cc1 -triple x86_64-linux -target-feature -sse2 -S -o /dev/null -verify %s
 // REQUIRES: x86-registered-target
 
-double f1(void) { // expected-error {{SSE2 register return with SSE2 disabled}}
+double f1(void) { // expected-warning {{SSE2 register return with SSE2 disabled}}
   return 1.4;
 }
 extern double g;
-void f2(void) { // expected-error {{SSE2 register return with SSE2 disabled}}
+void f2(void) { // expected-warning {{SSE2 register return with SSE2 disabled}}
   g = f1();
 }
 void take_double(double);
@@ -15,6 +15,6 @@
 }
 
 double return_double();
-void call_double(double *a) { // expected-error {{SSE2 register return with SSE2 disabled}}
+void call_double(double *a) { // expected-warning {{SSE2 register return with SSE2 disabled}}
   *a = return_double();
 }
Index: clang/test/CodeGen/x86_64-mno-sse.c
===
--- clang/test/CodeGen/x86

[PATCH] D102995: errorUnsupported should be non-fatal

[MJ via Phabricator via cfe-commits]

majiang31312 updated this revision to Diff 347546.
majiang31312 added a comment.

Try Arc


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102995

Files:
  clang/test/CodeGen/x86_64-mno-sse.c
  clang/test/CodeGen/x86_64-mno-sse2.c
  llvm/lib/Target/X86/X86ISelLowering.cpp


Index: llvm/lib/Target/X86/X86ISelLowering.cpp
===
--- llvm/lib/Target/X86/X86ISelLowering.cpp
+++ llvm/lib/Target/X86/X86ISelLowering.cpp
@@ -96,8 +96,8 @@
 static void errorUnsupported(SelectionDAG &DAG, const SDLoc &dl,
  const char *Msg) {
   MachineFunction &MF = DAG.getMachineFunction();
-  DAG.getContext()->diagnose(
-  DiagnosticInfoUnsupported(MF.getFunction(), Msg, dl.getDebugLoc()));
+  DAG.getContext()->diagnose(DiagnosticInfoUnsupported(
+  MF.getFunction(), Msg, dl.getDebugLoc(), DS_Warning));
 }
 
 X86TargetLowering::X86TargetLowering(const X86TargetMachine &TM,
Index: clang/test/CodeGen/x86_64-mno-sse2.c
===
--- clang/test/CodeGen/x86_64-mno-sse2.c
+++ clang/test/CodeGen/x86_64-mno-sse2.c
@@ -1,11 +1,11 @@
 // RUN: %clang_cc1 -triple x86_64-linux -target-feature -sse2 -S -o /dev/null 
-verify %s
 // REQUIRES: x86-registered-target
 
-double f1(void) { // expected-error {{SSE2 register return with SSE2 disabled}}
+double f1(void) { // expected-warning {{SSE2 register return with SSE2 
disabled}}
   return 1.4;
 }
 extern double g;
-void f2(void) { // expected-error {{SSE2 register return with SSE2 disabled}}
+void f2(void) { // expected-warning {{SSE2 register return with SSE2 disabled}}
   g = f1();
 }
 void take_double(double);
@@ -15,6 +15,6 @@
 }
 
 double return_double();
-void call_double(double *a) { // expected-error {{SSE2 register return with 
SSE2 disabled}}
+void call_double(double *a) { // expected-warning {{SSE2 register return with 
SSE2 disabled}}
   *a = return_double();
 }
Index: clang/test/CodeGen/x86_64-mno-sse.c
===
--- clang/test/CodeGen/x86_64-mno-sse.c
+++ clang/test/CodeGen/x86_64-mno-sse.c
@@ -1,15 +1,14 @@
 // RUN: %clang_cc1 -triple x86_64-linux -target-feature -sse -target-feature 
-sse2 -S -o /dev/null -verify %s
 // REQUIRES: x86-registered-target
 
-double f1(void) { // expected-error {{SSE register return with SSE disabled}}
+double f1(void) { // expected-warning {{SSE register return with SSE disabled}}
   return 1.4;
 }
 extern double g;
-void f2(void) { // expected-error {{SSE register return with SSE disabled}}
+void f2(void) { // expected-warning {{SSE register return with SSE disabled}}
   g = f1();
 }
 void take_double(double);
 void pass_double(void) {
-  // FIXME: Still asserts.
-  //take_double(1.5);
+  take_double(1.5);
 }


Index: llvm/lib/Target/X86/X86ISelLowering.cpp
===
--- llvm/lib/Target/X86/X86ISelLowering.cpp
+++ llvm/lib/Target/X86/X86ISelLowering.cpp
@@ -96,8 +96,8 @@
 static void errorUnsupported(SelectionDAG &DAG, const SDLoc &dl,
  const char *Msg) {
   MachineFunction &MF = DAG.getMachineFunction();
-  DAG.getContext()->diagnose(
-  DiagnosticInfoUnsupported(MF.getFunction(), Msg, dl.getDebugLoc()));
+  DAG.getContext()->diagnose(DiagnosticInfoUnsupported(
+  MF.getFunction(), Msg, dl.getDebugLoc(), DS_Warning));
 }
 
 X86TargetLowering::X86TargetLowering(const X86TargetMachine &TM,
Index: clang/test/CodeGen/x86_64-mno-sse2.c
===
--- clang/test/CodeGen/x86_64-mno-sse2.c
+++ clang/test/CodeGen/x86_64-mno-sse2.c
@@ -1,11 +1,11 @@
 // RUN: %clang_cc1 -triple x86_64-linux -target-feature -sse2 -S -o /dev/null -verify %s
 // REQUIRES: x86-registered-target
 
-double f1(void) { // expected-error {{SSE2 register return with SSE2 disabled}}
+double f1(void) { // expected-warning {{SSE2 register return with SSE2 disabled}}
   return 1.4;
 }
 extern double g;
-void f2(void) { // expected-error {{SSE2 register return with SSE2 disabled}}
+void f2(void) { // expected-warning {{SSE2 register return with SSE2 disabled}}
   g = f1();
 }
 void take_double(double);
@@ -15,6 +15,6 @@
 }
 
 double return_double();
-void call_double(double *a) { // expected-error {{SSE2 register return with SSE2 disabled}}
+void call_double(double *a) { // expected-warning {{SSE2 register return with SSE2 disabled}}
   *a = return_double();
 }
Index: clang/test/CodeGen/x86_64-mno-sse.c
===
--- clang/test/CodeGen/x86_64-mno-sse.c
+++ clang/test/CodeGen/x86_64-mno-sse.c
@@ -1,15 +1,14 @@
 // RUN: %clang_cc1 -triple x86_64-linux -target-feature -sse -target-feature -sse2 -S -o /dev/null -verify %s
 // REQUIRES: x86-registered-target
 
-double f1(void) { // expected-error {

[PATCH] D102995: errorUnsupported should be non-fatal

[MJ via Phabricator via cfe-commits]

majiang31312 added a comment.

In D102995#2778674 , @craig.topper 
wrote:

> "fatal" in the comment means don't diagnose any additional errors and 
> immediately stop. We attempt to recover to detect more errors, but the 
> emitted binary code is likely incorrect. I don't think we can just emit a 
> warning.
>
> The function name says "error" in its name, it should be an error.

Thanks for the explanations, it's reasonable now. 
Although “SSE register return with SSE disabled” seems strange as the code did 
not use SSE directly, it's another problem.  I'll close this one now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102995

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

[PATCH] D102995: errorUnsupported should be non-fatal

[MJ via Phabricator via cfe-commits]

majiang31312 added a comment.

Thanks for the information. 
Well, the key problem is x64 abi. But we can do better here for sure. 
First of all, we can fallback to use fpu instructions. I can get the following 
binary which should work correctly(after this fix, of course)

   :
 0:   55  push   %rbp
 1:   48 89 e5mov%rsp,%rbp
 4:   d9 ee   fldz
 6:   5d  pop%rbp
 7:   c3  retq
 8:   0f 1f 84 00 00 00 00nopl   0x0(%rax,%rax,1)
 f:   00
  
  0010 :
10:   55  push   %rbp
11:   48 89 e5mov%rsp,%rbp
14:   48 83 ec 20 sub$0x20,%rsp
18:   31 c0   xor%eax,%eax
1a:   c7 45 fc 00 00 00 00movl   $0x0,-0x4(%rbp)
21:   89 45 ecmov%eax,-0x14(%rbp)
24:   e8 00 00 00 00  callq  29 
  25: R_X86_64_PLT32  test-0x4
29:   dd 5d f0fstpl  -0x10(%rbp)

Although it might be incompatible with other libraries because of ABI problems, 
it is the responsibility of the linker to prevent possible errors. The compiler 
should just produce the binary as requested.  
There have been similar situations in https://reviews.llvm.org/D70465.

Second,  even if we decided not to support the fallback feature for the moment, 
we certainly could give a more clear message (maybe  something like 
"float/double arguments under -mno-sse are not supported yet").
In all, using float/double is not a error in code(even with -mno-sse) in my 
opinion.

However, to make -mno-sse(with float/double) working gracefully need quite a 
work, so I agree it's reasonable to keep current state as there are no serious 
users yet. Thanks again for the clarification!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102995

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