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

[flang] Fix build with AVOID_NATIVE_UINT128_T #63712

Closed
h-vetinari opened this issue Jul 6, 2023 · 20 comments · Fixed by llvm/llvm-project-release-prs#691
Closed

[flang] Fix build with AVOID_NATIVE_UINT128_T #63712

h-vetinari opened this issue Jul 6, 2023 · 20 comments · Fixed by llvm/llvm-project-release-prs#691

Comments

@h-vetinari
Copy link
Contributor

In trying to get flang usable in the conda-forge ecosystem (think of it as a cross-platform quasi-distribution; see #60730), One of the main remaining problems is that building and running flang against the MSVC runtime fails with a linkage error for the symbol __udivti3, which is in compiler-rt but not the MSVC runtime.

Linking to compiler-rt has its own set of problems, though, so I perked up when I stumbled over the following context for __udivti3:

Hans Wennborg: Sharing in case anyone else runs into the same problem: This caused Chromium builds to fail on Windows due to not linking against compiler-rt's builtins library. It turns out this patch caused us to compile some code doing 128-bit arithmetic and calling the __udivti3 runtime function. We worked around it by defining _LIBCPP_HAS_NO_INT128 until we can make the builtins library part of all our builds.

Mark de Wever: Interesting that this caused it. I expect the real cause is std::to_char for 128-bit values, there divisions are used. Does that mean the flag in __config is not set correctly? It was recently updated in D134912.

Even though flang doesn't use libcxx, I followed down this 128-bit rabbit hole a bit, and after some searching I found AVOID_NATIVE_UINT128_T. However, even having built flang with that (plus 21bff9c), it still runs into missing __udivti3 when trying to build a hello-world example.

I ended up asking if building flang without native 128-bit types considered supported, and got the following response:

@vzakhari: I do not think building with AVOID_NATIVE_UINT128_T is being tested or/and is fully functional. I remember trying to experimentally enable it would fail the runtime compilation because of the missing type conversion operators. The most reliable way to make sure that AVOID_NATIVE_UINT128_T works is to have at least one buildbot that uses it for Flang build and also runs some end-to-end testing. It seems to worth a separate issue.

Hence this issue. I don't know how hard it would be to set up an extra build bot, but that sounds like something that would be really helpful on platforms without native 128bit types, of which windows is a prime example.

@klausler
Copy link
Contributor

klausler commented Jul 6, 2023

It is not clear here whether the failure is a failure to link a compiler together or a failure to link a compiled program.

@h-vetinari
Copy link
Contributor Author

I've worked around the failure to link the compiler together (it's fine for me if that needs complier-rt), it's about the failure for linking an absolutely minimal hello world project.

@klausler
Copy link
Contributor

klausler commented Jul 6, 2023

See the patch in https://reviews.llvm.org/D154660. Does it make progress on this problem for you?

@llvmbot
Copy link
Collaborator

llvmbot commented Jul 6, 2023

@llvm/issue-subscribers-flang-runtime

@h-vetinari
Copy link
Contributor Author

See the patch in https://reviews.llvm.org/D154660. Does it make progress on this problem for you?

Thanks a lot for the quick response! I've built this (on top of flang 16), but at runtime, I still get:

>flang-new -flang-experimental-exec hello_world.f90
FortranDecimal.lib(binary-to-decimal.cpp.obj) : error LNK2019: unresolved external symbol __udivti3 referenced in function "public: __cdecl Fortran::decimal::BigRadixFloatingPointNumber<64,16>::BigRadixFloatingPointNumber<64,16>(class Fortran::decimal::BinaryFloatingPointNumber<64>,enum Fortran::decimal::FortranRounding)" (??0?$BigRadixFloatingPointNumber@$0EA@$0BA@@decimal@Fortran@@QEAA@V?$BinaryFloatingPointNumber@$0EA@@12@W4FortranRounding@12@@Z)

IIUC, the patch is still a clear improvement, but maybe it doesn't yet capture all uses of 128bit types?

@klausler
Copy link
Contributor

klausler commented Jul 7, 2023

Do you have to work with such an old revision? You should see better results with llvm-project/main.

@h-vetinari
Copy link
Contributor Author

Do you have to work with such an old revision?

Haha, it's the last released version. 😅

I get that flang is moving quickly though, so I'm looking forward to 17.0.0rc1. I could build everything for a given commit from main, but it'd be quite a hassle due to the way we need to build things separately (not least because public CI is too puny to do a full build of LLVM+clang+mlir+openmp+flang).

Ideally, I was hoping to be raising bugs I found from trying to compile scipy with flang so that they'd have a chance to be tackled in time for 17.0.x, but I guess bug reports based on flang 16 would also not be that useful...?

I guess rc1 is only 3 weeks away, so it might make sense to just wait a bit?

@klausler
Copy link
Contributor

klausler commented Jul 7, 2023

If you know of bugs, please report them. Bug reports aren't getting a lot of attention, but unreported bugs get none.

@h-vetinari
Copy link
Contributor Author

h-vetinari commented Jul 7, 2023

It's not a lack of willingness to report bugs. But to report bugs, it's a prerequisite to get flang 16 (or the required subset of LLVM for a given commit) working within our infrastructure - which I've been trying to do for quite a while (see #60730) -, in order to throw it at scipy1 and see what comes of it.

Footnotes

  1. or any of our other Fortran repos. But scipy is the most urgent for us - for various reasons we absolutely need to get flang working on it this year. Luckily it's mostly old Fortran code

@h-vetinari
Copy link
Contributor Author

h-vetinari commented Jul 28, 2023

Do you have to work with such an old revision? You should see better results with llvm-project/main.

In anticipation of the impending 17.0.0-rc1 tag, I've prepared our infra and built it consistently for a single commit from main (a4f4d82). When I apply your patch to that and build flang, I still get a missing symbol for a hello-world example:

>flang-new hello_world.f90
FortranDecimal.lib(binary-to-decimal.cpp.obj) : error LNK2019: unresolved external symbol __udivti3 referenced in function "public: __cdecl Fortran::decimal::BigRadixFloatingPointNumber<64,16>::BigRadixFloatingPointNumber<64,16>(class Fortran::decimal::BinaryFloatingPointNumber<64>,enum Fortran::decimal::FortranRounding)" (??0?$BigRadixFloatingPointNumber@$0EA@$0BA@@decimal@Fortran@@QEAA@V?$BinaryFloatingPointNumber@$0EA@@12@W4FortranRounding@12@@Z)
a.exe : fatal error LNK1120: 1 unresolved externals

@h-vetinari
Copy link
Contributor Author

When I apply your patch to that and build flang, I still get a missing symbol for a hello-world example:

To be fair, this might not be a case of AVOID_NATIVE_UINT128_T not working, but rather an occurrence of #25679

@h-vetinari
Copy link
Contributor Author

Thanks @klausler for merging https://reviews.llvm.org/D154660!

WDYT about backporting this to 17.x? Should not be too invasive I think...? If you agree, could you add it to the milestone?

@klausler
Copy link
Contributor

klausler commented Sep 5, 2023

Go ahead if you want to. I don't have any time for this.

@h-vetinari
Copy link
Contributor Author

Yeah, I can take care of this, except I don't have the rights to add this to the milestone, and the bot will fail without it:

@h-vetinari
Copy link
Contributor Author

/cherry-pick 1c35c1a

@llvmbot
Copy link
Collaborator

llvmbot commented Sep 5, 2023

/cherry-pick 1c35c1a

Error: Command failed due to missing milestone.

@EugeneZelenko
Copy link
Contributor

/cherry-pick 1c35c1a

@h-vetinari h-vetinari changed the title [flang] Fully test & support AVOID_NATIVE_UINT128_T [flang] Fix build with AVOID_NATIVE_UINT128_T Sep 5, 2023
@llvmbot
Copy link
Collaborator

llvmbot commented Sep 5, 2023

/branch llvm/llvm-project-release-prs/issue63712

@llvmbot
Copy link
Collaborator

llvmbot commented Sep 5, 2023

/pull-request llvm/llvm-project-release-prs#691

@h-vetinari
Copy link
Contributor Author

@tru
JFYI, I don't consider this mission-critical for 17.0.0, but it would be good to get it into the 17.0.x series at least.

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

Successfully merging a pull request may close this issue.

4 participants