Skip to content

reduced size of ValueFlow::Value #6784

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

Merged
firewave merged 8 commits into from
Dec 16, 2024
Merged

reduced size of ValueFlow::Value #6784

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f232413
config.h: added `FORCE_WARNING_*` macros
firewave Sep 8, 2024
057f766
vfvalue.h: enabled `-Wpadded` for this file and explicitly pad `Value…
firewave Sep 20, 2024
4a9c54c
vfvalue.h: re-arranged members to reduce excess padding
firewave Sep 2, 2024
26a474a
vfvalue.h: added TODOs
firewave Sep 2, 2024
22315df
vfvalue.h: reduced size of `indirect`
firewave Sep 20, 2024
2133354
vfvalue.h: reduced size of booleans
firewave Sep 20, 2024
1077ad2
vfvalue.h: reordered members to avoid explicit padding
firewave Sep 20, 2024
ed47405
vfvalue.h: mitigated `premium-misra-cpp-2023-12.2.1` selfcheck warning
firewave Dec 14, 2024
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
9 changes: 9 additions & 0 deletions lib/config.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,20 +182,29 @@ static const std::string emptyString;
#define SUPPRESS_WARNING_GCC_POP
#define SUPPRESS_WARNING_CLANG_PUSH(warning) SUPPRESS_WARNING_PUSH(warning)
#define SUPPRESS_WARNING_CLANG_POP SUPPRESS_WARNING_POP
#define FORCE_WARNING_PUSH(warn) _Pragma("clang diagnostic push") _Pragma(STRINGISIZE(clang diagnostic warning warn))
#define FORCE_WARNING_CLANG_PUSH(warning) FORCE_WARNING_PUSH(warning)
#define FORCE_WARNING_CLANG_POP SUPPRESS_WARNING_POP
#elif defined(__GNUC__)
#define SUPPRESS_WARNING_PUSH(warning) _Pragma("GCC diagnostic push") _Pragma(STRINGISIZE(GCC diagnostic ignored warning))
#define SUPPRESS_WARNING_POP _Pragma("GCC diagnostic pop")
#define SUPPRESS_WARNING_GCC_PUSH(warning) SUPPRESS_WARNING_PUSH(warning)
#define SUPPRESS_WARNING_GCC_POP SUPPRESS_WARNING_POP
#define SUPPRESS_WARNING_CLANG_PUSH(warning)
#define SUPPRESS_WARNING_CLANG_POP
#define FORCE_WARNING_PUSH(warning)
#define FORCE_WARNING_CLANG_PUSH(warning)
#define FORCE_WARNING_CLANG_POP
#else
#define SUPPRESS_WARNING_PUSH(warning)
#define SUPPRESS_WARNING_POP
#define SUPPRESS_WARNING_GCC_PUSH(warning)
#define SUPPRESS_WARNING_GCC_POP
#define SUPPRESS_WARNING_CLANG_PUSH(warning)
#define SUPPRESS_WARNING_CLANG_POP
#define FORCE_WARNING_PUSH(warning)
#define FORCE_WARNING_CLANG_PUSH(warning)
#define FORCE_WARNING_CLANG_POP
#endif

#if !defined(NO_WINDOWS_SEH) && defined(_WIN32) && defined(_MSC_VER)
Expand Down
4 changes: 4 additions & 0 deletions lib/vfvalue.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@
namespace ValueFlow {
Value::Value(const Token *c, long long val, Bound b)
: bound(b),
safe(false),
conditional(false),
macro(false),
defaultArg(false),
intvalue(val),
varvalue(val),
condition(c) {
Expand Down
103 changes: 58 additions & 45 deletions lib/vfvalue.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
#include <type_traits>
#include <vector>

FORCE_WARNING_CLANG_PUSH("-Wpadded")

class Token;

namespace ValueFlow
Expand All @@ -43,6 +45,10 @@ namespace ValueFlow

explicit Value(long long val = 0, Bound b = Bound::Point) :
bound(b),
safe(false),
conditional(false),
macro(false),
defaultArg(false),
intvalue(val),
varvalue(val),
wideintvalue(val)
Expand Down Expand Up @@ -262,6 +268,53 @@ namespace ValueFlow
/** The value bound */
Bound bound = Bound::Point;

/** value relies on safe checking */
// cppcheck-suppress premium-misra-cpp-2023-12.2.1
bool safe : 1;

/** Conditional value */
bool conditional : 1;

/** Value is is from an expanded macro */
bool macro : 1;

/** Is this value passed as default parameter to the function? */
bool defaultArg : 1;

long long : 4; // padding

/** kind of moved */
enum class MoveKind : std::uint8_t { NonMovedVariable, MovedVariable, ForwardedVariable } moveKind = MoveKind::NonMovedVariable;

enum class LifetimeScope : std::uint8_t { Local, Argument, SubFunction, ThisPointer, ThisValue } lifetimeScope = LifetimeScope::Local;

enum class LifetimeKind : std::uint8_t {
// Pointer points to a member of lifetime
Object,
// A member of object points to the lifetime
SubObject,
// Lambda has captured lifetime(similar to SubObject)
Lambda,
// Iterator points to the lifetime of a container(similar to Object)
Iterator,
// A pointer that holds the address of the lifetime
Address
} lifetimeKind = LifetimeKind::Object;

/** How known is this value */
enum class ValueKind : std::uint8_t {
/** This value is possible, other unlisted values may also be possible */
Possible,
/** Only listed values are possible */
Known,
/** Inconclusive */
Inconclusive,
/** Listed values are impossible */
Impossible
} valueKind = ValueKind::Possible;

std::int8_t indirect{}; // TODO: can we reduce the size?
Copy link
Owner

@danmar danmar Dec 15, 2024

Choose a reason for hiding this comment

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

An old wisdom is that it is a good idea to order members by size.
See i.e.: http://katecpp.github.io/struct-members-order/
I have seen good results in a embedded project.
I guess it's pretty platform-dependent and would make no difference on some platforms.

would it be a good idea to use "pragma packed" on this struct?

Copy link
Collaborator Author

@firewave firewave Dec 15, 2024

Choose a reason for hiding this comment

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

An old wisdom is that it is a good idea to order members by size.

Interesting. I have not done much in this memory so far because there were always still bigger issues in the projects I worked on.

would it be a good idea to use "pragma packed" on this struct?

Given the low amount of explicit padding we have to apply (and my limited understanding of packed) that might not be necessary. I also think that might come only into play is when you really want to optimize for cachelines (which I have no experience working with) and I assume this will always be out-of-scope for this application.


/** int value (or sometimes bool value?) */
long long intvalue{};

Expand All @@ -279,35 +332,20 @@ namespace ValueFlow

ErrorPath errorPath;

ErrorPath debugPath;
ErrorPath debugPath; // TODO: make lighter by default
Copy link
Collaborator Author

@firewave firewave Sep 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is only used when --debug is specified. So we waste 16 bytes for having a full type instead of a pointer.

I did hack this out locally and it saves about 3% IR and several operations no longer show up at all in profiling it appears we just narrowly exceed some threshold for some compiler optimization(s) or allocation pattern.

This comment was marked as outdated.

Sign in to view


/** For calculated values - varId that calculated value depends on */
nonneg int varId{};

enum class UnknownFunctionReturn : uint8_t {
enum class UnknownFunctionReturn : std::uint8_t {
no, // not unknown function return
outOfMemory, // out of memory
outOfResources, // out of resource
other // other
};
UnknownFunctionReturn unknownFunctionReturn{UnknownFunctionReturn::no};

/** value relies on safe checking */
bool safe{};

/** Conditional value */
bool conditional{};

/** Value is is from an expanded macro */
bool macro{};

/** Is this value passed as default parameter to the function? */
bool defaultArg{};

int8_t indirect{};

/** kind of moved */
enum class MoveKind : std::uint8_t { NonMovedVariable, MovedVariable, ForwardedVariable } moveKind = MoveKind::NonMovedVariable;
long long : 24; // padding

/** Path id */
MathLib::bigint path{};
Expand All @@ -320,38 +358,11 @@ namespace ValueFlow
// Set to where a lifetime is captured by value
const Token* capturetok{};

enum class LifetimeKind : std::uint8_t {
// Pointer points to a member of lifetime
Object,
// A member of object points to the lifetime
SubObject,
// Lambda has captured lifetime(similar to SubObject)
Lambda,
// Iterator points to the lifetime of a container(similar to Object)
Iterator,
// A pointer that holds the address of the lifetime
Address
} lifetimeKind = LifetimeKind::Object;

enum class LifetimeScope : std::uint8_t { Local, Argument, SubFunction, ThisPointer, ThisValue } lifetimeScope = LifetimeScope::Local;

RET_NONNULL static const char* toString(MoveKind moveKind);
RET_NONNULL static const char* toString(LifetimeKind lifetimeKind);
RET_NONNULL static const char* toString(LifetimeScope lifetimeScope);
RET_NONNULL static const char* toString(Bound bound);

/** How known is this value */
enum class ValueKind : std::uint8_t {
/** This value is possible, other unlisted values may also be possible */
Possible,
/** Only listed values are possible */
Known,
/** Inconclusive */
Inconclusive,
/** Listed values are impossible */
Impossible
} valueKind = ValueKind::Possible;

void setKnown() {
valueKind = ValueKind::Known;
}
Expand Down Expand Up @@ -419,4 +430,6 @@ namespace ValueFlow
};
}

FORCE_WARNING_CLANG_POP

#endif // vfvalueH
Loading