This is an archive of the discontinued LLVM Phabricator instance.

[libc][math] Fix floating-point test support on x86_64 Apple machines
ClosedPublic

Authored by ddcc on Jun 26 2023, 5:20 PM.

Details

Summary

Provide platform-specific x87 FPU definitions and operations

Diff Detail

Event Timeline

ddcc created this revision.Jun 26 2023, 5:20 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 26 2023, 5:20 PM
ddcc requested review of this revision.Jun 26 2023, 5:20 PM
lntue added inline comments.Jun 26 2023, 6:26 PM
libc/src/__support/FPUtil/x86_64/FEnvImpl.h
624–640

Do you mind combining the #ifdef to make it a bit easier to read:

#ifdef __APPLE__
  x87_status.status_word |= (fpstate->status_word & 0x3F);
  // We can set the x87 control word as is as there no sensitive bits.
  x87_status.control_word = fpstate->control_word;
#else
  x87_status.status_word |= (fpstate->x87_status.status_word & 0x3F);
  // Copy other non-sensitive parts of the status word.
  for (int i = 0; i < 5; i++) {
    x87_status._[i] = fpstate->x87_status._[i];
  // We can set the x87 control word as is as there no sensitive bits.
  x87_status.control_word = fpstate->x87_status.control_word;
#endif // __APPLE__
ddcc updated this revision to Diff 534815.Jun 26 2023, 6:49 PM

Combine ifdef section

lntue accepted this revision.Jun 26 2023, 7:23 PM
This revision is now accepted and ready to land.Jun 26 2023, 7:23 PM
ddcc added inline comments.Jun 26 2023, 8:12 PM
libc/src/__support/FPUtil/x86_64/FEnvImpl.h
220–221

Just wanted to point out that this change will affect all x86_64 platforms. But based on the comment, I believe this function is supposed to have been fetching the exception status bits from bot hteh x87 FPU and the SSE unit anyway?

sivachandra added inline comments.Jun 27 2023, 10:09 AM
libc/src/__support/FPUtil/x86_64/FEnvImpl.h
220–221

Thanks for catching this! If the tests are passing, go ahead and submit. If you can actually craft a test case to catch regressions, even better.

ddcc added inline comments.Jun 30 2023, 12:56 AM
libc/src/__support/FPUtil/x86_64/FEnvImpl.h
220–221

Do you know if the pre-merge checks run the tests at all? I don't have a non-Apple x86_64 machine easily accessible

lntue added inline comments.Jun 30 2023, 8:56 AM
libc/src/__support/FPUtil/x86_64/FEnvImpl.h
220–221

I think the pre-merge checks are run on a Linux x86_64 machine, probably in overlay mode. Post-commit bots will run them in full build mode.

ddcc added inline comments.Jun 30 2023, 7:55 PM
libc/src/__support/FPUtil/x86_64/FEnvImpl.h
220–221

Ok, ran the tests in overlay mode locally on a Linux machine, and all passed.