This is an archive of the discontinued LLVM Phabricator instance.

Differential D83342

[test-suite][AIX] Fix C++ benchmark compatibility issues on AIX
ClosedPublic

Authored by stevewan on Jul 7 2020, 1:14 PM.

Details

Reviewers
hubert.reinterpretcast
daltenty
ShuhongL
Summary

This reimplements what has been reverted in 630313d.

The old implementation applied the X/OPEN compatibility flag to the entire miniFE benchmark. This caused a build failure with mytimer.cpp due to its usages of struct timezone which is not available on AIX when the said compatibility flag is set. This new fix now also defines "UseTimes" which enables another path in the application to avoid the usages of timezone.

Diff Detail

Event Timeline

stevewan created this revision.Jul 7 2020, 1:14 PM
Herald added a subscriber: mgorny. · View Herald TranscriptJul 7 2020, 1:14 PM
Harbormaster completed remote builds in B63286: Diff 276194.Jul 7 2020, 1:14 PM
stevewan edited the summary of this revision. (Show Details)Jul 7 2020, 1:15 PM
stevewan added reviewers: hubert.reinterpretcast, daltenty, ShuhongL.
hubert.reinterpretcast added inline comments.Jul 7 2020, 2:28 PM
MultiSource/Benchmarks/DOE-ProxyApps-C++/miniFE/CMakeLists.txt
5

Why not just append to CXXFLAGS, which already has macro defines?

stevewan marked an inline comment as done.Jul 8 2020, 11:05 AM
stevewan added inline comments.
MultiSource/Benchmarks/DOE-ProxyApps-C++/miniFE/CMakeLists.txt
5

That was the original fix in D82321. Doing so applies the flag to the entire miniFE benchmark, yet one of the files in the benchmark makes use of timezone which is not available when the X/OPEN mode is specified, and therefore results in build failures.

hubert.reinterpretcast added inline comments.Jul 8 2020, 11:18 AM
MultiSource/Benchmarks/DOE-ProxyApps-C++/miniFE/CMakeLists.txt
5

The breakage was identified on a non-AIX platform, and the fix now limits the change to AIX. You can continue appending to the list here.

stevewan marked an inline comment as done.Jul 8 2020, 11:35 AM
stevewan added inline comments.
MultiSource/Benchmarks/DOE-ProxyApps-C++/miniFE/CMakeLists.txt
5

Unfortunately it was also an uncaught breakage on AIX.

hubert.reinterpretcast added inline comments.Jul 8 2020, 12:10 PM
MultiSource/Benchmarks/DOE-ProxyApps-C++/miniFE/CMakeLists.txt
5

Does defining UseTimes and _XOPEN_SOURCE across the board for the build here work for AIX?

stevewan marked 4 inline comments as done.Jul 8 2020, 2:43 PM
stevewan added inline comments.
MultiSource/Benchmarks/DOE-ProxyApps-C++/miniFE/CMakeLists.txt
5

Yes. Thanks for the suggestion, I'll update this patch.

Do you mind give a brief explanation of what the UseTimes does? I tried to look for docs but didn't find any.

hubert.reinterpretcast added inline comments.Jul 8 2020, 3:10 PM
MultiSource/Benchmarks/DOE-ProxyApps-C++/miniFE/CMakeLists.txt
5

It's one of the "configuration macros" in the application.

stevewan updated this revision to Diff 276577.Jul 8 2020, 3:16 PM
stevewan marked an inline comment as done.

Take a different path in the application to avoid using 'timezone'.

Harbormaster completed remote builds in B63507: Diff 276577.Jul 8 2020, 3:16 PM
stevewan marked 2 inline comments as done.Jul 8 2020, 3:17 PM
stevewan added inline comments.
MultiSource/Benchmarks/DOE-ProxyApps-C++/miniFE/CMakeLists.txt
5

Got it, thanks!

hubert.reinterpretcast accepted this revision.Jul 8 2020, 3:21 PM

LGTM; thanks.

This revision is now accepted and ready to land.Jul 8 2020, 3:21 PM
stevewan edited the summary of this revision. (Show Details)Jul 8 2020, 3:30 PM
stevewan marked an inline comment as done.
stevewan closed this revision.Jul 9 2020, 10:45 AM

Committed in https://github.com/llvm/llvm-test-suite/commit/f0caeccfb893344d73474e9d8f6c034ae7f451f9.

Revision Contents

PathSize
MultiSource/
Benchmarks/
DOE-ProxyApps-C++/
miniFE/
7 lines

Diff 276577

MultiSource/Benchmarks/DOE-ProxyApps-C++/miniFE/CMakeLists.txt

list(APPEND CXXFLAGS -I. -DMINIFE_SCALAR=double -DMINIFE_LOCAL_ORDINAL=int -DMINIFE_GLOBAL_ORDINAL=int -DMINIFE_CSR_MATRIX -DREDSTORM) # -DREDSTORM for mkdir list(APPEND CXXFLAGS -I. -DMINIFE_SCALAR=double -DMINIFE_LOCAL_ORDINAL=int -DMINIFE_GLOBAL_ORDINAL=int -DMINIFE_CSR_MATRIX -DREDSTORM) # -DREDSTORM for mkdir
# Use X/OPEN compatibility flag on AIX to avoid problems with some
# versions of the system headers. Define "UseTimes" to avoid usages of
# "struct timezone" in some source files because it's not available on
# AIX when the compatibility flag is set.
hubert.reinterpretcastUnsubmitted
Done
ReplyInline Actions

Why not just append to CXXFLAGS, which already has macro defines?

hubert.reinterpretcast: Why not just append to `CXXFLAGS`, which already has macro defines?
stevewanAuthorUnsubmitted
Done
ReplyInline Actions

That was the original fix in D82321. Doing so applies the flag to the entire miniFE benchmark, yet one of the files in the benchmark makes use of timezone which is not available when the X/OPEN mode is specified, and therefore results in build failures.

stevewan: That was the original fix in D82321. Doing so applies the flag to the entire miniFE benchmark…
hubert.reinterpretcastUnsubmitted
Done
ReplyInline Actions

The breakage was identified on a non-AIX platform, and the fix now limits the change to AIX. You can continue appending to the list here.

hubert.reinterpretcast: The breakage was identified on a non-AIX platform, and the fix now limits the change to AIX.
stevewanAuthorUnsubmitted
Done
ReplyInline Actions

Unfortunately it was also an uncaught breakage on AIX.

stevewan: Unfortunately it was also an uncaught breakage on AIX.
hubert.reinterpretcastUnsubmitted
Done
ReplyInline Actions

Does defining UseTimes and _XOPEN_SOURCE across the board for the build here work for AIX?

hubert.reinterpretcast: Does defining `UseTimes` and `_XOPEN_SOURCE` across the board for the build here work for AIX?
stevewanAuthorUnsubmitted
Done
ReplyInline Actions

Yes. Thanks for the suggestion, I'll update this patch.

Do you mind give a brief explanation of what the UseTimes does? I tried to look for docs but didn't find any.

stevewan: Yes. Thanks for the suggestion, I'll update this patch. Do you mind give a brief explanation…
hubert.reinterpretcastUnsubmitted
Done
ReplyInline Actions

It's one of the "configuration macros" in the application.

hubert.reinterpretcast: It's one of the "configuration macros" in the application.
stevewanAuthorUnsubmitted
Done
ReplyInline Actions

Got it, thanks!

stevewan: Got it, thanks!
if(CMAKE_SYSTEM_NAME STREQUAL "AIX")
list(APPEND CXXFLAGS -D_XOPEN_SOURCE=700 -DUseTimes)
endif()
list(APPEND LDFLAGS -lm) list(APPEND LDFLAGS -lm)
set(RUN_OPTIONS -nx 64 -ny 64 -nz 64) set(RUN_OPTIONS -nx 64 -ny 64 -nz 64)
llvm_multisource(miniFE) llvm_multisource(miniFE)
target_include_directories(miniFE PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}) target_include_directories(miniFE PRIVATE ${CMAKE_CURRENT_SOURCE_DIR})