Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #26449 #26749

Closed
wants to merge 3 commits into from
Closed

Fixes #26449 #26749

wants to merge 3 commits into from

Conversation

gz
Copy link
Contributor

@gz gz commented Jul 3, 2015

Allows for compilation of libcore without floating point support.

thepowersgang and others added 3 commits July 2, 2015 21:50
This allows to compile libcore on architectures that lack floating point support
or to use libcore in kernel code without worrying about saving and restoring
the floating point register state inside the kernel.
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @huonw (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@huonw
Copy link
Member

huonw commented Jul 3, 2015

cc @rust-lang/libs, how do we want to handle this?

@Gankra
Copy link
Contributor

Gankra commented Jul 3, 2015

0_o computers are crazy

This feels like an RFC is needed, unless it lands behind a -Z flag. Also seems like more of a tools (compiler interface) issue assuming you accept that this is at all a desirable thing to do.

@gz
Copy link
Contributor Author

gz commented Jul 4, 2015

I would argue the change falls under:
Additions only likely to be noticed by other developers-of-rust, invisible to users-of-rust.
(citing from https://github.com/rust-lang/rfcs/).

But if you decide that it needs an RFC I can write one. Let me know.

@thepowersgang
Copy link
Contributor

Just to add to @gz's comment - This should only have an affect when libcore is being compiled manually, not when it's being used by general users. This means that it shouldn't need a -Z flag

@alexcrichton
Copy link
Member

I feel like this isn't quite the way we'd want to approach this. We generally have a fairly bad track record for sprinkling #[cfg] and having it be maintainable into the future. I'd prefer to have a more official solution in the compiler itself.

@gz
Copy link
Contributor Author

gz commented Jul 6, 2015

For solving it in the compiler: compiling libcore without SSE registers works on the nightly before #26025 happened -- so there may indeed be some LLVM changes that caused this strict requirement.
Also note that it is not enough to just compile libcore with SSE and not use any float in the kernel as LLVM will happily optimize other stuff with SSE registers then.

@thepowersgang
Copy link
Contributor

@alexcrichton The ideal solution would be to get --soft-float working once more, but it appears that LLVM was what broke this. Either that, or have a way to tell LLVM to not use SSE etc for anything except explicit floating-point operations.

What sort of compiler-provided solution would work for disabling floating point support, without causing compile errors?

@alexcrichton
Copy link
Member

@gz is this just a bug that needs to be fixed in upstream LLVM then?

@thepowersgang what is --soft-float? Has this ever been an argument to the compiler?

What sort of compiler-provided solution would work for disabling floating point support, without causing compile errors?

I don't understand how floating point support is connected to the SSE errors that have been reported in #26449, so I fear that this may be going a bit far down the rabbit hole.

@thepowersgang
Copy link
Contributor

@alexcrichton Sorry, it's -C soft-float - It's supposed to disable any attempt at using hardware floating point, but since the most recent LLVM update it hasn't (leading to #26449 on x86_64)

From what I have been told, the x86_64 ELF ABI requires that floating-point arguments be passed in SSE registers, which is why floating point support requires SSE to be enabled. I assume that until recently -C soft-float broke the ABI and used general purpose registers instead.

@alexcrichton
Copy link
Member

Perhaps the solution is then to dig around in LLVM rather than adding a compile mode to libcore?

@bors
Copy link
Contributor

bors commented Jul 13, 2015

☔ The latest upstream changes (presumably #26981) made this pull request unmergeable. Please resolve the merge conflicts.

@Gankra
Copy link
Contributor

Gankra commented Jul 27, 2015

@alexcrichton should we close this?

@alexcrichton
Copy link
Member

Yeah I think that we may want to end up tackling this a different way, perhaps only stabilizing a large portion of the functionality in the standard library instead of libcore, or perhaps allowing libcore to be recompiled in an easier fashion.

@Tyler-Hardin
Copy link

@alexcrichton, would you reconsider patching libcore to allow disabling float support? This is adding a lot of manual steps to thepowersgang's barebones kernel build, requiring getting, patching (usually merging by hand), and rebuilding a modified libcore w/ every Rust update.

It seems to me that float support in libcore is a good idea in the general case. Given the correct initialization, it can be done in a freestanding environment, so it makes sense to include it. But, at the same time, there are use cases that don't want to do that initialization. Given it would require breaking the ABI (specifically, the calling convention) in LLVM and, that by all normal standards, SSE support is guaranteed on amd64, I doubt we'll see a fix from LLVM. (Nonetheless, I've posted to their development list to get their thoughts on it.)

Perhaps the solution is then to dig around in LLVM rather than adding a compile mode to libcore?

// If this is x86-64, and we disabled SSE, we can't return FP values,
// or SSE or MMX vectors.
if ((ValVT == MVT::f32 || ValVT == MVT::f64 ||
     VA.getLocReg() == X86::XMM0 || VA.getLocReg() == X86::XMM1) &&
      (Subtarget->is64Bit() && !Subtarget->hasSSE1())) {
  report_fatal_error("SSE register return with SSE disabled");
}

In the method X86TargetLowering::LowerReturn. The problem is that it's technically correct functionality. The only possible workaround would be for them to invent with their own nonstandard calling convention for amd64. Both GCC and Clang return the same error when trying to use floats in amd64 w/ SSE disabled, even with software floats enabled.

Test case (gcc/clang -mno-sse -msoft-float test.c):

double foo() { return 0.0; }
int main() { foo(); return 0; }

(With and without -msoft-float, the result is the same.) So it doesn't seem this error will be going away. Out of the options:

  • Removing f32/64 from libcore and moving it to std: This would screw anyone doing freestanding in an environment that supports floats.
  • Leaving it as is: Screws kernel devs.
  • Add cfg option: Everybody's happy!(?)

cc @gankro

@Gankra
Copy link
Contributor

Gankra commented Aug 23, 2015

I'm uh... Not really sure why I was pinged on this. I agree that llvm is a jerk who makes our lives hard?

@Tyler-Hardin
Copy link

@gankro, sorry. I assumed you were the libs guy, I guess. :P Not familiar enough with the project who would be best to contact.

@alexcrichton
Copy link
Member

Unfortunately I don't think I understand the problem space here enough to be able to say one way or another what the best way forward is. I have personally built kernels in Rust in the past and they've all run just fine, I've never run into these kinds of floating point problems, so I don't know what this is precisely targeting.

Regardless, though, I'm not a huge fan of sprinkling around #[cfg] annotations for somewhat obscure builds of libcore, it's incredibly difficult to maintain those over time to make sure they all keep working.

@gz
Copy link
Contributor Author

gz commented Aug 24, 2015

@alexcrichton While you can definitely build a kernel that makes use of SSE registers, it is usually not desirable to do so, as you want to minimize the number of registers saved on context switches as much as possible. Especially with wide registers (such as SSE) because they may have higher access latencies than general purpose register. Also note that just disabling SSE in the build and not using float/double is not good enough since llvm usually tries to make good use of those registers to optimize other stuff in libcore.

There was a point in time where LLVM was able to compile libcore without SSE and floating point support. There is currently an open bug for LLVM that seems to target the same issue (for C++), so it may be better to wait and see how this gets resolved: https://llvm.org/bugs/show_bug.cgi?id=23203

@Tyler-Hardin
Copy link

@gz, I think the fact that both gcc and llvm have the same "SSE return while SSE disabled" error suggests that the change that caused this was a bug fix rather than a removal of functionality. It used to result in a miscompilation, including SSE when SSE is disabled, which (as of the bug fix) they catch and throw an error. If I get some time this weekend I'll try building libcore <1.2 with SSE disabled and check the disassembly for SSE instructions. (This was the point of the GGC/LLVM section in my last post, but I forgot to get around to making it explicit. Sorry if you already got it and I'm just being overly verbose now.)

@gz
Copy link
Contributor Author

gz commented Aug 24, 2015

@Tyler-Hardin I see. In case it helps with your research, the nightly before #26025 was the last version known to work. (See also #26449)

@Tyler-Hardin
Copy link

@gz Well, this was unexpected (for me, at least), but it did actually have working soft floats on amd64. I've modified an old version of @thepowersgang's barebones to do some floating point math, disassembled it, and verified that is had call to __muldf3@PLT (a builtin for multiplication of soft floats) and no mention of xmm (SSE registers) whatsoever. I even ran it on qemu amd64 and it worked. FWIW, I tested with #26000.

So, the above would make it sound like an LLVM regression. But, I used --emit=llvm-ir and tested the LLVM with a my system llc (3.6.2), using only the -soft-float option, and it had functioning soft floats (same as above). This really doesn't make any sense. My best guess is that the -C soft-float option passed to Rust doesn't make it to the LLVM backend, but at this point it seems like it's going to take some familiarity with Rust's codebase, so I'm lost.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants