Take math-errno into account with '#pragma float_control(precise,on)' and 'attribute__((optnone)).
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/include/clang/Basic/FPOptions.def | ||
---|---|---|
30 | Does this mean MathErrno is tracked in both LangOpts and FPOptions? | |
clang/lib/CodeGen/CGBuiltin.cpp | ||
2314 | You should add a comment here explaining what this is doing. I don't think it's obvious apart from the description of this patch. | |
clang/lib/CodeGen/CGCall.cpp | ||
2472 | I don't understand what this is doing. | |
clang/lib/CodeGen/CodeGenModule.h | ||
617 | Why is this a module level flag, and why does it default to true? | |
clang/test/CodeGen/math-errno.c | ||
3 | Isn't math-errno true by default when fast-math isn't used? | |
50 | I think the 'afn' flag here is a problem. The backend has no concept of errno, so 'afn' will be treated as allowing the function to be replaced. |
clang/test/CodeGen/math-errno.c | ||
---|---|---|
34 | Can you add a runline with -O0. That should prevent all instances of the intrinsics, right? |
clang/lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
2315 | Since we handle "#pragma float_control" for all targets, we should be consistent. I don't think the reference to MSVC here is useful. I'm not sure why MSVC thinks it needs to call the sqrtf() function at O1, but I don't necessarily think we should follow that lead. If we generate the intrinsic here, the optimizer will make its own decisions about O1 versus O2, etc. There's no reason that errno should be preserved at O1 if it's been disabled with -fno-math-errno or we're enabled fast-math. | |
2329 | I'm confused by this condition. If MathErrnoOverride is true, don't we need to set errno regardless of the optimization level or fast-math setting? | |
2332 | Why do we need to check FastMath if the function is marked as optnone? | |
2347 | Based on the comment above, if the function call is marked as const, then it will never set errno, so I don't think we need to check EmitIntrinsic in that case. | |
clang/test/CodeGen/math-errno.c | ||
29 | Shouldn't the precise pragma remove the nofpcalss(nan inf) attributes? The definition of setFPPreciseEnabled looks like it's trying to do that. Maybe we need another patch to handle that? | |
41 | Again, this may be beyond the scope of your change, but it seems like '#pragma float_control(precise, off) should lead to fast-math flags being set on this call. | |
44 | I expected the intrinsic to be called in this case. Do you know why it isn't? I believe it is without your change. | |
47 | I didn't expect to see the intrinsic in this case. If we use the intrinsic, the x86-backend will generate sqrtss instead of the function call, and I think that is wrong. | |
57 | Again, I didn't expect the intrinsic with optnone in any of these cases. |
clang/test/CodeGen/math-errno.c | ||
---|---|---|
3 | Yes, that's correct. This RUN line simulates compilation at -O2. The following cc1 options are triggered: -fmath-errno -ffp-contract=on | |
29 |
I will make a note of it and work on it in a subsequent patch. | |
41 |
Yes. Making a note of it. |
clang/include/clang/Basic/FPOptions.def | ||
---|---|---|
29–30 | Shouldn't this one be Float16ExcessPrecision? (Are we missing test coverage that would have caught that?) | |
clang/include/clang/Basic/LangOptions.h | ||
858 | Everything else does !Value; is it intentional that you're using Value instead? | |
clang/lib/CodeGen/CGBuiltin.cpp | ||
2351–2358 | I think this has gotten sufficiently complex that it might be worth splitting the logic out a bit more: bool Optimize = FD->hasAttr<ConstAttr>() && !ErrnoOverriden && !OptNone; if (!Optimize) Optimize = FD->hasAttr<ConstAttr>() && !ErrnoOverriden && !OptNone; ... and so on ... WDYT? |
clang/include/clang/Basic/FPOptions.def | ||
---|---|---|
29–30 | Yup -- that can be handled in a separate patch as it doesn't really have much to do with this one -- just something I noticed as a drive-by. | |
clang/include/clang/Basic/LangOptions.h | ||
858 | Hmm, okay. Making sure I understand the logic, please excuse me if this is a dumb question. :-) So the idea here is that if fp_precise is on, we have to honor math errno, and if fp_precise is off, we can ignore math errno? | |
clang/lib/CodeGen/CGBuiltin.cpp | ||
2326 | Is the part about -O2 accurate? We just check not -O0 in the code. |
clang/include/clang/Basic/LangOptions.h | ||
---|---|---|
858 | Correct. |
clang/lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
2351–2358 | I understand I need to split the logic, but I don't get your proposal here. Do you mean? bool Optimize = FD->hasAttr<ConstAttr>() && !ErrnoOverriden && !OptNone; if (!Optimize) Optimize = !getLangOpts().MathErrno && !ErrnoOverriden && !OptNone; ... and so on ... |
clang/lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
2351–2358 | Oops, yeah, that a was copy pasta mistake on my part. You've got the right idea -- mostly just split up the logic and add comments explaining why the predicates exist. |
clang/lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
2351–2358 | Changed the name of the variable you proposed. Is that OK? |
clang/lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
2351–2358 | Totally fine by me, but there's some more logic that you can strip out of the if statement, and it'd help me out if there were some comments explaining why each of these blocks might disable generating intrinsics. |
clang/lib/CodeGen/CGBuiltin.cpp | ||
---|---|---|
2348–2350 |
clang/docs/UsersManual.rst | ||
---|---|---|
1726–1727 | Yeah, that's not the end of the world (if it was touching the whole file, that should be done as an NFC change outside of this patch, but these changes are related enough I think it's fine to do in this patch). |
@aaron.ballman thanks for the reviews. Added some note in the RN, let me know if that is enough.
@andrew.w.kaylor Would you mind taking a last look at this before I can push this, please? I got the thumbs up from Aaron.
This breaks check-clang on macOS: http://45.33.8.238/macm1/68762/step_7.txt
Please take a look and revert for now if it takes a while to fix.
Hi @zahiraam ,
I have a couple of downstream testcases that fail with this patch.
Before
> clang bbi-86364.c -lm -O3 > ./a.out
passed but with the patch the assert in the program fails:
a.out: bbi-86364.c:9: int main(): Assertion `(*__errno_location ()) == 33' failed.
Is this as expected?
This seems unexpected to me and it seems to relate to whether you include errno.h or not: https://godbolt.org/z/EPWzazx9r -- @zahiraam do you have ideas as to what's going on?
I haven't looked at it as I saw that the comment has been deleted. Let me look into it.
Oh, it's not the inclusion of errno.h that matters, it's the declaration of __errno_location: https://godbolt.org/z/zo4PaPEME -- it seems that inclusion of __attribute__ ((__const__)) is what makes the distinction: https://godbolt.org/z/1bePhvaG4
Yes confirming this. In the errno.h header the function is declared: extern int *errno_location (void) attribute__ ((nothrow )) attribute ((const));
Since we can't really change the header, this needs to be changed in the compiler?
I think the issue comes from here: https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGBuiltin.cpp#L2365
[...]
I think the issue comes from here: https://github.com/llvm/llvm-project/blob/main/clang/lib/CodeGen/CGBuiltin.cpp#L2365
This looks like it's happening only for -Oi (i>=1) and on Linux only?
Yes I haven't seen it with -O0. I also haven't seen it for my out-of-tree target, only for linux/X86.