Page MenuHomePhabricator

[FPEnv] Document requirement of function attributes with constrained floating point
ClosedPublic

Authored by kpn on Sep 20 2019, 9:19 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

kpn created this revision.Sep 20 2019, 9:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 20 2019, 9:19 AM

I agree with marking function call sites as "strictfp".

But I'm wondering how you came up with this particular list for function definitions? In particular, I'm not sure whether noimplicitfloat is really needed. I understand this flag is supposed to prevent unintended use of floating-point registers (e.g. for Linux kernel code). You don't really need to prevent that for constrained floating point semantics, I think. (You do need to prevent introduction of floating-point instruction that might cause spurious exception, but e.g. introducing moves through FP registers should still be fine.)

As to "strictfp" at the function definition, does this actually do anything currently? I cannot find any LLVM code that checks for this. The documentation also doesn't really say.

kpn added a comment.Sep 24 2019, 12:17 PM

I agree with marking function call sites as "strictfp".

But I'm wondering how you came up with this particular list for function definitions? In particular, I'm not sure whether noimplicitfloat is really needed. I understand this flag is supposed to prevent unintended use of floating-point registers (e.g. for Linux kernel code). You don't really need to prevent that for constrained floating point semantics, I think. (You do need to prevent introduction of floating-point instruction that might cause spurious exception, but e.g. introducing moves through FP registers should still be fine.)

I saw it mentioned in a conversation on the lists at some point. From reading the thread (which I don't have in hand, sorry!) I didn't see anyone disagree. Of course, if it wasn't important enough then maybe there was no good reason to bother at the time. *shrug*

Are we certain that there is no architecture important to LLVM that will ever trap when simply doing a move of arbitrary bytes using FP instructions?

Anybody?

Also, the documentation doesn't say in what circumstances the lack of noimplicitfloat will allow floating point to be introduced. I'm checking on that now. I'm seeing the attribute used in tests with names like "popcnt.ll", so I'm inclined to lean conservative for now at least.

As to "strictfp" at the function definition, does this actually do anything currently? I cannot find any LLVM code that checks for this. The documentation also doesn't really say.

I'm certain that I've seen it mentioned in the context of the inliner, but I don't think anyone has done anything to the inliner yet. I admit I'm unclear on other requirements for it.

Are we certain that there is no architecture important to LLVM that will ever trap when simply doing a move of arbitrary bytes using FP instructions?

strictfp should be enough to imply that the compiler shouldn't implicitly insert FP operations that can raise an exception, on any target. As it happens, there is one reasonably popular target that can raise an exception on an FP load: x86 without SSE, using x87 floating-point. But noimplicitfloat shouldn't be necessary there... strictfp should be enough to imply that can't insert x87 loads that aren't immediately used by an arithmetic operation. (This is probably broken, currently, and probably nobody will ever fix it, but that isn't really relevant.) No other modern target has similar behavior.

OK, then I'd suggest we just add "strictfp" to the definition, with the semantics that codegen for this function must not introduce any potentially trapping FP instructions (that were not already in the source IR) -- the latter property should probably also be documented under the "strictfp" entry.

It was probably me that argued for the strictfp attribute being required on the function definition. My reasoning was that when the inliner wants to inline a function containing strict FP calls into a function that does not, it would use the strictfp attribute as a cue to convert all of the FP operations in the target function to constrained. This can, of course, be deduced from the IR, but if we treat the attribute on the definition as optional then the inliner would always need to check the IR anyway. So if it isn't required it has limited usefulness.

I agree with Eli that strictfp should provide enough information to prohibit introducing FP instructions that might raise exceptions, and that's probably a better argument for requiring the strictfp attribute on function definitions.

kpn updated this revision to Diff 221804.Sep 25 2019, 10:42 AM

Update for review comments. Eliminate verbiage about noimplicitfloat. Add mention of strictfp preventing the addition of potentially trapping instructions.

andrew.w.kaylor accepted this revision.Sep 25 2019, 11:54 AM

This looks good to me.

This revision is now accepted and ready to land.Sep 25 2019, 11:54 AM
kpn added a comment.Sep 25 2019, 11:57 AM

I agree with marking function call sites as "strictfp".

Say, would this apply to all function call sites? Or just to ones that aren't intrinsics?

In D67839#1683038, @kpn wrote:

I agree with marking function call sites as "strictfp".

Say, would this apply to all function call sites? Or just to ones that aren't intrinsics?

Hmm, I hadn't thought about intrinsics. It probably doesn't matter much -- intrinsics usually already have well-defined semantics, that really cannot be changed by some attribute either. (In particular, the constrained intrinsics already specify their exception semantics; what additional information would adding a strictfp attribute provide?)

In D67839#1683038, @kpn wrote:

I agree with marking function call sites as "strictfp".

Say, would this apply to all function call sites? Or just to ones that aren't intrinsics?

I think all.

We've got a problem yet to solve with architecture-specific intrinsics. There are a lot of X86-intrinsics that perform floating point operations. I expect other architecture have the same issue. I doubt that the optimizer attempts to transform many of them, but we'll need to do something when we get to ISel. In theory we could create constrained versions of all of them, but it might be better to just mark them as "strictfp" and have ISel make conservative assumptions. We could attach the local rounding mode and exception behavior as metadata if that would be helpful.

@pengfei , do you think the "strictfp" attribute on an intrinsic would tell you enough for X86 instruction selection? I think for any x86 instructions that take rounding mode operands the intrinsic would already have that explicitly specified, right?

@pengfei , do you think the "strictfp" attribute on an intrinsic would tell you enough for X86 instruction selection? I think for any x86 instructions that take rounding mode operands the intrinsic would already have that explicitly specified, right?

I think the exception attribute on architecture-specific intrinsics should accept option ignore/maytrap/strict like the constrained intrinsics do. "strictfp" is not enough if we want to attach exception attribute to intrinsic.
For rounding mode, AFAIK, some SSE/AVX intrinsics may implicitly use MXCSR.RC, e.g. __m128i _mm_cvtpd_epi32 (__m128d a), AVX512 intrinsics as well as partial SSE/AVX intrinsics can be specified through parameter, but they still may use MAXCSR.RC if specified with _MM_FROUND_CUR_DIRECTION. Should we attach the rounding attribute to intrinsic, if we had explicitly specified MXCSR.RC?

This revision was automatically updated to reflect the committed changes.

This is failing on the sphinx build bot:
lab.llvm.org:8011/builders/llvm-sphinx-docs

kpn added a comment.Sep 30 2019, 7:55 AM

This is failing on the sphinx build bot:
lab.llvm.org:8011/builders/llvm-sphinx-docs

This should be fixed now. Sorry about the breakage! How do I kick off the llvm-sphinx-docs builder?