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.

Sign up for GitHub

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

a few c++ fixes to allow compilation on Windows #2282

Merged
merged 6 commits into from
Oct 11, 2023
Merged

a few c++ fixes to allow compilation on Windows #2282

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bdef7ad
a few c++ fixes to allow compilation on Windows
apwojcik Oct 3, 2023
ba2a9a2
incorporate code changes from the review feedback
apwojcik Oct 4, 2023
03b03f4
Merge branch 'develop' into windows_cxx_compilation
apwojcik Oct 4, 2023
e599b93
incorporate review feedback
apwojcik Oct 7, 2023
761e977
Merge branch 'develop' into windows_cxx_compilation
apwojcik Oct 7, 2023
5bafb63
Merge branch 'develop' into windows_cxx_compilation
causten Oct 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/api/api.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
#include <migraphx/register_op.hpp>
#include <migraphx/json.hpp>
#include <migraphx/convert_to_json.hpp>
#include <array>
#include <algorithm>
#include <cstdarg>
namespace migraphx {
Expand Down
1 change: 1 addition & 0 deletions src/api/include/migraphx/migraphx.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

#include <stdlib.h>
#include <stdbool.h>
#include <stdint.h>

#include <migraphx/api/export.h>

Expand Down
2 changes: 1 addition & 1 deletion src/api/include/migraphx/migraphx.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ template <class PrivateMigraphTypeNameProbe>
std::string compute_type_name()
{
std::string name;
#ifdef _MSC_VER
#if defined(_MSC_VER) && !defined(__clang__)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why does it requires check for clang. Aren't both exclusive if it is MSVC, it can't be clang.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Clang compiler on Windows is running in MSVC compatibility mode. It sets _MSC_VER, too. So, only checking _MSC_VER is not exclusive and is insufficient to differentiate between native MSVC and Clang. See detailed information https://clang.llvm.org/docs/MSVCCompatibility.html

name = typeid(PrivateMigraphTypeNameProbe).name();
name = name.substr(7);
#else
Expand Down
1 change: 1 addition & 0 deletions src/driver/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
#include <migraphx/json.hpp>
#include <migraphx/version.h>

#include <migraphx/instruction.hpp>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where are we using the instruction class in this file? I cant seem to find it.

#include <migraphx/dead_code_elimination.hpp>
#include <migraphx/eliminate_identity.hpp>
#include <migraphx/eliminate_pad.hpp>
Expand Down
7 changes: 3 additions & 4 deletions src/include/migraphx/auto_register.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,9 @@ const int auto_register<Action, T>::static_register = auto_register_action<Actio
#define MIGRAPHX_AUTO_REGISTER_NAME_DETAIL(x) migraphx_auto_register_##x
#define MIGRAPHX_AUTO_REGISTER_NAME(x) MIGRAPHX_AUTO_REGISTER_NAME_DETAIL(x)
// NOLINTNEXTLINE
#define MIGRAPHX_AUTO_REGISTER(...) \
void MIGRAPHX_AUTO_REGISTER_NAME(__LINE__)(migraphx::auto_register<__VA_ARGS__> x = \
migraphx::auto_register<__VA_ARGS__>{}) \
__attribute__((unused));
#define MIGRAPHX_AUTO_REGISTER(...) \
[[maybe_unused]] void MIGRAPHX_AUTO_REGISTER_NAME(__LINE__)( \
migraphx::auto_register<__VA_ARGS__> x = migraphx::auto_register<__VA_ARGS__>{});

} // namespace MIGRAPHX_INLINE_NS
} // namespace migraphx
Expand Down
4 changes: 4 additions & 0 deletions src/include/migraphx/config.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@

#include <migraphx/export.h>

#if defined(_MSC_VER) && !defined(__clang__)
#include <ciso646>
pfultz2 marked this conversation as resolved.
Show resolved Hide resolved
#endif

#if !defined(MIGRAPHX_USE_CLANG_TIDY) && !defined(DOXYGEN)

#ifdef BUILD_DEV
Expand Down
11 changes: 11 additions & 0 deletions src/include/migraphx/filesystem.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,17 @@
#if defined(CPPCHECK)
#define MIGRAPHX_HAS_FILESYSTEM 1
#define MIGRAPHX_HAS_FILESYSTEM_TS 1
#elif defined(_WIN32)
#if _MSC_VER >= 1920
#define MIGRAPHX_HAS_FILESYSTEM 1
#define MIGRAPHX_HAS_FILESYSTEM_TS 0
#elif _MSC_VER >= 1900
#define MIGRAPHX_HAS_FILESYSTEM 0
#define MIGRAPHX_HAS_FILESYSTEM_TS 1
#else
#define MIGRAPHX_HAS_FILESYSTEM 0
#define MIGRAPHX_HAS_FILESYSTEM_TS 0
#endif
#elif defined(__has_include)
#if __has_include(<filesystem>) && __cplusplus >= 201703L
#define MIGRAPHX_HAS_FILESYSTEM 1
Expand Down
3 changes: 0 additions & 3 deletions src/include/migraphx/float_equal.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,9 +27,6 @@
#include <algorithm>
#include <cmath>
#include <numeric>
#ifdef _MSC_VER
#include <iso646.h>
#endif

#include <migraphx/requires.hpp>
#include <migraphx/config.hpp>
Expand Down
4 changes: 2 additions & 2 deletions src/include/migraphx/generate.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ constexpr T normalize(unsigned long z)
template <class T, MIGRAPHX_REQUIRES(is_signed<T>{} and not is_floating_point<T>{})>
constexpr T normalize(unsigned long z)
{
const auto max = 1UL << (sizeof(T) * 5);
const auto max = 1ULL << (sizeof(T) * 5);
const auto half_max = max / 2;
return half_max - (z % max);
}
Expand All @@ -58,7 +58,7 @@ template <class T,
not std::is_same<T, bool>{})>
constexpr T normalize(unsigned long z)
{
const auto max = 1UL << (sizeof(T) * 5);
const auto max = 1ULL << (sizeof(T) * 5);
return z % max;
}

Expand Down
7 changes: 7 additions & 0 deletions src/include/migraphx/matcher.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
#include <migraphx/type_name.hpp>
#include <migraphx/source_location.hpp>
#include <migraphx/config.hpp>
#include <array>
#include <unordered_map>
#include <unordered_set>

Expand Down Expand Up @@ -307,6 +308,12 @@ struct matcher_result
{
}

instruction_container& operator=(const std::unordered_map<std::string, instruction_ref>& x)
umangyadav marked this conversation as resolved.
Show resolved Hide resolved
{
ins_map = x;
return *this;
}

instruction_ref operator[](const std::string& name) const
{
auto it = ins_map.find(name);
Expand Down
1 change: 1 addition & 0 deletions src/include/migraphx/op/nonmaxsuppression.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#ifndef MIGRAPHX_GUARD_OPERATORS_NONMAXSUPPRESSION_HPP
#define MIGRAPHX_GUARD_OPERATORS_NONMAXSUPPRESSION_HPP

#include <array>
#include <cmath>
#include <queue>
#include <cstdint>
Expand Down
1 change: 1 addition & 0 deletions src/include/migraphx/op/roialign.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
#include <migraphx/dfor.hpp>
#include <migraphx/ranges.hpp>
#include <migraphx/shape_for_each.hpp>
#include <array>
#include <cmath>
#include <numeric>
#include <utility>
Expand Down
11 changes: 11 additions & 0 deletions src/include/migraphx/optional.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,17 @@
#if defined(CPPCHECK)
#define MIGRAPHX_HAS_OPTIONAL 1
#define MIGRAPHX_HAS_OPTIONAL_TS 1
#elif defined(_WIN32)
#if _MSC_VER >= 1920
#define MIGRAPHX_HAS_OPTIONAL 1
#define MIGRAPHX_HAS_OPTIONAL_TS 0
#elif _MSC_VER >= 1900
#define MIGRAPHX_HAS_OPTIONAL 0
#define MIGRAPHX_HAS_OPTIONAL_TS 1
#else
#define MIGRAPHX_HAS_OPTIONAL 0
#define MIGRAPHX_HAS_OPTIONAL_TS 0
#endif
#elif defined(__has_include)
#if __has_include(<optional>) && __cplusplus >= 201703L
#define MIGRAPHX_HAS_OPTIONAL 1
Expand Down
1 change: 1 addition & 0 deletions src/include/migraphx/run_loop.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
#include <migraphx/module.hpp>
#include <migraphx/config.hpp>
#include <migraphx/ranges.hpp>
#include <array>
#include <string>

namespace migraphx {
Expand Down
1 change: 1 addition & 0 deletions src/include/migraphx/source_location.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#ifndef MIGRAPHX_GUARD_MIGRAPHX_SOURCE_LOCATION_HPP
#define MIGRAPHX_GUARD_MIGRAPHX_SOURCE_LOCATION_HPP

#include <cstdint>
#include <migraphx/config.hpp>

#if defined(CPPCHECK)
Expand Down
2 changes: 1 addition & 1 deletion src/include/migraphx/type_name.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ template <class PrivateMigraphTypeNameProbe>
std::string compute_type_name()
{
std::string name;
#ifdef _MSC_VER
#if defined(_MSC_VER) && !defined(__clang__)
name = typeid(PrivateMigraphTypeNameProbe).name();
name = name.substr(7);
#else
Expand Down
2 changes: 1 addition & 1 deletion src/onnx/onnx_parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ void onnx_parser::parse_from(std::istream& is, std::string name)
this->filename = std::move(name);
auto parent_path = fs::path(this->filename).parent_path();
if(not parent_path.empty())
this->path = parent_path;
this->path = parent_path.string();

onnx::ModelProto model;
if(model.ParseFromIstream(&is))
Expand Down
1 change: 1 addition & 0 deletions src/targets/cpu/include/migraphx/cpu/pointwise.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
#ifndef MIGRAPHX_GUARD_AMDMIGRAPHX_CPU_POINTWISE_HPP
#define MIGRAPHX_GUARD_AMDMIGRAPHX_CPU_POINTWISE_HPP

#include <array>
#include <migraphx/config.hpp>
#include <migraphx/context.hpp>
#include <migraphx/check_shapes.hpp>
Expand Down
1 change: 1 addition & 0 deletions src/targets/gpu/hiprtc/main.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
#include <migraphx/msgpack.hpp>
#include <migraphx/file_buffer.hpp>
#include <migraphx/ranges.hpp>
#include <array>
#include <iostream>
#include <cstring>

Expand Down
2 changes: 1 addition & 1 deletion test/include/test.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,7 +333,7 @@ enum class color
bg_blue = 44,
bg_default = 49
};
inline std::ostream& operator<<(std::ostream& os, const color& c)
inline std::ostream& operator<<(std::ostream& os, [[maybe_unused]] const color& c)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer a (void)c added under #ifdef _WIN32, so we can still get diagnostics for unused variable on the linux side.

{
#ifndef _WIN32
static const bool use_color = isatty(STDOUT_FILENO) != 0;
Expand Down
1 change: 0 additions & 1 deletion test/multi_target/multitarget_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@
#include <migraphx/make_op.hpp>
#include <migraphx/check_shapes.hpp>
#include <migraphx/functional.hpp>
#include <basic_ops.hpp>
#include <migraphx/compile_options.hpp>
#include <migraphx/register_target.hpp>
#include <migraphx/generate.hpp>
Expand Down
1 change: 1 addition & 0 deletions test/promote_literals_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
#include <migraphx/program.hpp>
#include <migraphx/dead_code_elimination.hpp>
#include <migraphx/eliminate_common_subexpression.hpp>
#include <migraphx/instruction.hpp>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We dont use the instruction class here either, at least as far as I can tell. If windows has problems with incomplete types then maybe we should add this header in instruction_ref.hpp under #ifdef _WIN32

#include <migraphx/pass_manager.hpp>
#include <migraphx/make_op.hpp>
#include <migraphx/serialize.hpp>
Expand Down
1 change: 1 addition & 0 deletions tools/api/api.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
#include <migraphx/register_op.hpp>
#include <migraphx/json.hpp>
#include <migraphx/convert_to_json.hpp>
#include <array>
#include <algorithm>
#include <cstdarg>
namespace migraphx {
Expand Down
1 change: 1 addition & 0 deletions tools/api/migraphx.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@

#include <stdlib.h>
#include <stdbool.h>
#include <stdint.h>

#include <migraphx/api/export.h>

Expand Down