This is an archive of the discontinued LLVM Phabricator instance.

[X86] Don't allow floating-point return types when SSE[12] is disabled
ClosedPublic

Authored by rnk on Dec 7 2016, 7:24 AM.

Details

Summary

The following program hits a fatal_error in the X86 backend, when the
program is compiled with -mno-sse or -mno-sse2, which is understandable
due to the calling convention:

float f() { return 0.5f; };

since the error occurs in the backend, there are stack traces and bug
report messages that are generated.

This patch allows clang to avoid crashing and report a proper diagnostic.

No tests are included in this patch, since it was already tested in llvm's codegen test suite.

Diff Detail

Event Timeline

thegameg updated this revision to Diff 80594.Dec 7 2016, 7:24 AM
thegameg retitled this revision from to [X86] Don't allow floating-point return types when SSE[12] is disabled.
thegameg updated this object.
thegameg added a subscriber: llvm-commits.
RKSimon edited edge metadata.Dec 7 2016, 7:50 AM

Please can you add the context to the diff?

Please can you add the context to the diff?

I'm sorry, I'm not sure I understand what you mean by "context". Should I include the comments in the .patch? Should I add the comments added in the revision as comments in the code?

Please can you add the context to the diff?

I'm sorry, I'm not sure I understand what you mean by "context". Should I include the comments in the .patch? Should I add the comments added in the revision as comments in the code?

http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

Use svn diff --diff-cmd=diff -x -U999999 to create patches that include surrounding context so its easier to work out how the diff fits into the rest of the source.

Cheers, Simon

thegameg updated this revision to Diff 80607.Dec 7 2016, 9:00 AM
thegameg edited edge metadata.

More context added, thanks RKSimon.

joerg edited edge metadata.Dec 23 2016, 1:11 PM

There are two issues here. First of all, it is missing a test case. The second issue is that I somewhat don't like the local diagnose_fatal_error definition, I wonder if SDAG should provide such a helper in general.

thegameg added a comment.EditedDec 26 2016, 2:52 AM

There are two issues here. First of all, it is missing a test case.

There are already tests on this, but now it shows up in clang as just a diagnostic instead of a crash.

nosse-error1.ll
nosse-error2.ll

The second issue is that I somewhat don't like the local diagnose_fatal_error definition, I wonder if SDAG should provide such a helper in general.

I see. Should I just inline the code for each call?

Also, should FastISel be handling this too ?

RKSimon resigned from this revision.Jan 6 2017, 6:10 AM
RKSimon removed a reviewer: RKSimon.
RKSimon added a subscriber: RKSimon.
davide added a subscriber: davide.Apr 5 2017, 11:03 AM
rnk commandeered this revision.May 11 2017, 3:36 PM
rnk added a reviewer: thegameg.
rnk added a subscriber: rnk.

I'll clean this up and land it with a clang-side test. We get too many issues about this.

rnk updated this revision to Diff 98692.May 11 2017, 3:37 PM
  • clean up, add test
This revision was automatically updated to reflect the committed changes.