Page MenuHomePhabricator

[X86] Fix wrong handle with "-mno-x87"
Needs ReviewPublic

Authored by LiuChen3 on Wed, Apr 7, 11:28 PM.

Details

Summary

When the '-mno-x87' command line option is used, it should prevent all generation
of x87 instructions. Currently there are several circumstances under which it does not.
For example: https://godbolt.org/z/qnPaW7dd6

This patch adds "HasX87" predicates to each X87 instructions and when 80-bit
float pointer is supposed to pass or return on the x87 stack, reporting
an error if X87 is not supported.

Diff Detail

Event Timeline

LiuChen3 created this revision.Wed, Apr 7, 11:28 PM
LiuChen3 requested review of this revision.Wed, Apr 7, 11:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Apr 7, 11:28 PM
LiuChen3 retitled this revision from [X86] Fix wrong handle wih "-mno-x87" to [X86] Fix wrong handle with "-mno-x87".

What about cases that use x87 to do float/double conversions? I think it’s i64 conversions in 32-bit mode. But maybe other cases. Does gcc disable those with -mno-80387?

What about cases that use x87 to do float/double conversions? I think it’s i64 conversions in 32-bit mode. But maybe other cases. Does gcc disable those with -mno-80387?

You are right! GCC can do the conversions: https://godbolt.org/z/87ez838oc .
With this patch, llvm will fail in ISEL. I want to make another patch to do this.

What about cases that use x87 to do float/double conversions? I think it’s i64 conversions in 32-bit mode. But maybe other cases. Does gcc disable those with -mno-80387?

You are right! GCC can do the conversions: https://godbolt.org/z/87ez838oc .
With this patch, llvm will fail in ISEL. I want to make another patch to do this.

I'm more concerned about these cases that don't use long double and aren't missed constant folding. https://godbolt.org/z/qGv54ef34

I'm more concerned about these cases that don't use long double and aren't missed constant folding. https://godbolt.org/z/qGv54ef34

I see. This also break the ABI. Even when returning fp80 on 32bit target, gcc does not report an error. Can llvm report an error just like on a 64bit target?
Should we discuss the ABI issue on llvm-dev?

What about cases that use x87 to do float/double conversions? I think it’s i64 conversions in 32-bit mode. But maybe other cases. Does gcc disable those with -mno-80387?

You are right! GCC can do the conversions: https://godbolt.org/z/87ez838oc .
With this patch, llvm will fail in ISEL. I want to make another patch to do this.

I'm more concerned about these cases that don't use long double and aren't missed constant folding. https://godbolt.org/z/qGv54ef34

It seems not easy to handle this. I want fix a bug in this patch. For this part, It looks more like a feature. We want to do that in another patch. What's your opinion here?

What about cases that use x87 to do float/double conversions? I think it’s i64 conversions in 32-bit mode. But maybe other cases. Does gcc disable those with -mno-80387?

You are right! GCC can do the conversions: https://godbolt.org/z/87ez838oc .
With this patch, llvm will fail in ISEL. I want to make another patch to do this.

I'm more concerned about these cases that don't use long double and aren't missed constant folding. https://godbolt.org/z/qGv54ef34

It seems not easy to handle this. I want fix a bug in this patch. For this part, It looks more like a feature. We want to do that in another patch. What's your opinion here?

What is the usage model you’re trying to enable? This has been broken for a long time. Why is it important now?

Does adding HasX87 predicates to the instructions cause the conversions in my example to fail?

Does adding HasX87 predicates to the instructions cause the conversions in my example to fail?

Yes, it is. But this fail appeared before isel because of breaking ABI.

What is the usage model you’re trying to enable? This has been broken for a long time. Why is it important now?

I don't know any specific case for now. This was found by @andrew.w.kaylor. I will check with him.