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

[clapack] Remove broken host-arithchk #17573

Merged
merged 1 commit into from
Jun 11, 2021

Conversation

ras0219-msft
Copy link
Contributor

@ras0219-msft ras0219-msft commented Apr 29, 2021

As part of host dependencies, I moved arithchk to be built for the host and executed for the target. Unfortunately that's not how arithchk works. arithchk must be built for the target platform, because the detected information is fundamentally baked in at compile time.

I've lifted the pre-generated files from #17311 to handle all our official targets, plus added an triplet extension point "CLAPACK_ARITH_PATH" which will allow cross-compiling users to supply their own custom arith.h based on their system information. The logic is:

  1. If the user supplied arith.h, use that.
  2. If we're native-targeting, run arithchk.
  3. If we're cross-targeting a known platform, use that arith.h
  4. Otherwise, print a warning and run arithchk. This is likely to fail in the case of android, but it would succeed for "minor variants" like arm64-linux-dynamic.

As a bonus, this significantly improves compile times for clapack, since it no longer requires the host build.

@Neumann-A @cenit @szhorvat I'd appreciate any input you could provide as users of numeric libraries.

@szhorvat
Copy link
Contributor

szhorvat commented Apr 29, 2021

Just a warning that I did not have access to ARM Windows machines when generating the arith.h headers. My best guess was that there would be no difference between Windows 32-bit ARM and 32-bit x86, as well as Windows 64-bit ARM and 64-bit x86, but it is still a guess. The macOS headers are correct (generated on the correct architecture).

The Linux one is not from my PR. The NO_LONG_LONG looks suspicious though. I thought this should only be printed if sizeof(long long) is not 8, which would be rather unusual on any modern platform ... It certainly doesn't happen on a Rasperry Pi with 32-bit Linux.

Unfortunately, it seems that f2c hasn't been maintained for a long time, which means that there are no guarantees that everything works as it should with it even on newer platforms ... All I can say is that igraph's tests pass.

@ras0219-msft
Copy link
Contributor Author

I generated the linux header via a 64-bit docker container, so at least it's what is output by arithchk on my machine.

@szhorvat
Copy link
Contributor

szhorvat commented Apr 29, 2021

Something's not right there. On Linux x86_64, I get:

#define IEEE_8087
#define Arith_Kind_ASL 1
#define Long int
#define Intcast (int)(long)
#define Double_Align
#define X64_bit_pointers
#define QNaN0 0x0
#define QNaN1 0xfff80000

This is with gcc 7.5.

I do not have direct access to Linux on aarch64 (only through CI like Travis).

@ras0219-msft
Copy link
Contributor Author

ras0219-msft commented Apr 29, 2021

It seems like the official CMake buildsystem (http://www.netlib.org/clapack/clapack-3.2.1-CMAKE.tgz) sets NO_LONG_LONG on arithchk.c:

set_target_properties(arithchk PROPERTIES COMPILE_DEFINITIONS 
  "NO_FPINIT;NO_LONG_LONG")
ADD_CUSTOM_COMMAND(
   OUTPUT ${CMAKE_CURRENT_BINARY_DIR}/arith.h
   COMMAND arithchk > ${CMAKE_CURRENT_BINARY_DIR}/arith.h
   DEPENDS arithchk
   )

arithchk itself the passes that through:

#ifndef NO_LONG_LONG
		if (sizeof(long long) < 8)
#endif
			fprintf(f, "#define NO_LONG_LONG\n");

It looks like the Makefile first tries to compile without it, then falls back to defining it?

arith.h: arithchk.c
	$(CC) $(CFLAGS) -DNO_FPINIT arithchk.c -lm ||\
	 $(CC) -DNO_LONG_LONG $(CFLAGS) -DNO_FPINIT arithchk.c -lm
	./a.out >arith.h
	rm -f a.out arithchk.o

The only use of NO_LONG_LONG I can find in the actual code is in sysdep1.h and controls 64-bit file support.

@ras0219-msft ras0219-msft force-pushed the dev/roschuma/clapack branch from b19e0bf to ce08bfb Compare April 29, 2021 19:04
@szhorvat
Copy link
Contributor

That explains it. We didn't use the clapack distribution you link to in igraph. Instead, we re-translated LAPACK 3.5.0 with f2c according to the instructions there. I did not do this myself though so I do not know the details. I think the translation may not have succeeded for more recent versions (??).

@PhoebeHui PhoebeHui added info:internal This PR or Issue was filed by the vcpkg team. category:port-bug The issue is with a library, which is something the port should already support labels Apr 30, 2021
@PhoebeHui PhoebeHui added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Jun 9, 2021
@ras0219-msft
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@strega-nil-ms
Copy link
Contributor

Thanks robert

@strega-nil-ms strega-nil-ms merged commit c0bca71 into microsoft:master Jun 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support info:internal This PR or Issue was filed by the vcpkg team. requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants