Skip to content

Commit b15e201

Browse files
asudarsaasudarsa
committed
Addressed the following comments:
1. LLVM standard coding violations pointed out. 2. Removed 'Ty' field in the SYCLPropertyValue class and adjusted the code. Signed-off-by: Arvind Sudarsanam <[email protected]>
1 parent 4de3158 commit b15e201

File tree

3 files changed

+83
-110
lines changed

3 files changed

+83
-110
lines changed

‎llvm/include/llvm/Support/SYCLPropertySetIO.h‎

Lines changed: 47 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -54,7 +54,7 @@ class SYCLPropertyValue {
5454
using SizeTy = uint64_t;
5555

5656
// Defines supported property types
57-
enum Type { first = 0, NONE = first, UINT32, BYTE_ARRAY, last = BYTE_ARRAY };
57+
enum Type { first = 0, None = first, UInt32, ByteArray, last = ByteArray };
5858

5959
// Translates C++ type to the corresponding type tag.
6060
template <typename T> static Type getTypeTag();
@@ -69,18 +69,24 @@ class SYCLPropertyValue {
6969
~SYCLPropertyValue() {}
7070

7171
SYCLPropertyValue() = default;
72-
SYCLPropertyValue(Type T) : Ty(T) {}
7372

74-
SYCLPropertyValue(uint32_t Val) : Ty(UINT32), Val({Val}) {}
73+
SYCLPropertyValue(Type Ty) {
74+
if (Ty == UInt32)
75+
Val = (uint32_t)0;
76+
else if (Ty == ByteArray)
77+
Val = (std::byte *)(0);
78+
else
79+
llvm_unreachable_internal("unsupported SYCL property type");
80+
}
81+
SYCLPropertyValue(uint32_t Val) : Val({Val}) {}
7582
SYCLPropertyValue(const std::byte *Data, SizeTy DataBitSize);
7683
template <typename C, typename T = typename C::value_type>
7784
SYCLPropertyValue(const C &Data)
7885
: SYCLPropertyValue(reinterpret_cast<const std::byte *>(Data.data()),
7986
Data.size() * sizeof(T) * CHAR_BIT) {}
8087
SYCLPropertyValue(const llvm::StringRef &Str)
8188
: SYCLPropertyValue(reinterpret_cast<const std::byte *>(Str.data()),
82-
Str.size() * sizeof(char) *
83-
/* bits in one byte */ 8) {}
89+
Str.size() * sizeof(char) * CHAR_BIT) {}
8490
SYCLPropertyValue(const SYCLPropertyValue &P);
8591
SYCLPropertyValue(SYCLPropertyValue &&P);
8692

@@ -90,13 +96,14 @@ class SYCLPropertyValue {
9096

9197
// Get property value as unsigned 32-bit integer
9298
uint32_t asUint32() const {
93-
assert((Ty == UINT32) && "must be UINT32 value");
99+
assert(std::holds_alternative<uint32_t>(Val) && "must be a uint32_t value");
94100
return std::get<uint32_t>(Val);
95101
}
96102

97103
// Get raw data size in bits.
98104
SizeTy getByteArraySizeInBits() const {
99-
assert((Ty == BYTE_ARRAY) && "must be BYTE_ARRAY value");
105+
assert(std::holds_alternative<std::byte *>(Val) &&
106+
"must be a byte array value");
100107
SizeTy Res = 0;
101108

102109
for (size_t I = 0; I < sizeof(SizeTy); ++I) {
@@ -121,30 +128,33 @@ class SYCLPropertyValue {
121128

122129
// Get byte array data including the leading bytes encoding the size.
123130
const std::byte *asRawByteArray() const {
124-
assert((Ty == BYTE_ARRAY) && "must be BYTE_ARRAY value");
131+
assert(std::holds_alternative<std::byte *>(Val) &&
132+
"must be a byte array value");
125133
auto *ByteArrayVal = std::get<std::byte *>(Val);
126134
return ByteArrayVal;
127135
}
128136

129137
// Get byte array data excluding the leading bytes encoding the size.
130138
const std::byte *asByteArray() const {
131-
assert((Ty == BYTE_ARRAY) && "must be BYTE_ARRAY value");
139+
assert(std::holds_alternative<std::byte *>(Val) &&
140+
"must be a byte array value");
132141

133142
auto ByteArrayVal = std::get<std::byte *>(Val);
134143
return ByteArrayVal + sizeof(SizeTy);
135144
}
136145

137-
bool isValid() const { return getType() != NONE; }
146+
bool isValid() const { return getType() != None; }
138147

139-
// Set property value when data type is UINT32_T
148+
// Set property value when data type is uint32_t
140149
void set(uint32_t V) {
141-
assert((Ty == UINT32) && "must be UINT32 value");
150+
assert(std::holds_alternative<uint32_t>(Val) && "must be a uint32_t value");
142151
Val = V;
143152
}
144153

145-
// Set property value when data type is BYTE_ARRAY
154+
// Set property value when data type is 'std::byte *'
146155
void set(std::byte *V, int DataSize) {
147-
assert((Ty == BYTE_ARRAY) && "must be BYTE_ARRAY value");
156+
assert(std::holds_alternative<std::byte *>(Val) &&
157+
"must be a byte array value");
148158
size_t DataBitSize = DataSize * CHAR_BIT;
149159
constexpr size_t SizeFieldSize = sizeof(SizeTy);
150160
// Allocate space for size and data.
@@ -161,23 +171,25 @@ class SYCLPropertyValue {
161171
std::memcpy(ByteArrayVal + SizeFieldSize, V, DataSize);
162172
}
163173

164-
Type getType() const { return Ty; }
174+
Type getType() const {
175+
if (std::holds_alternative<uint32_t>(Val))
176+
return UInt32;
177+
if (std::holds_alternative<std::byte *>(Val))
178+
return ByteArray;
179+
return None;
180+
}
165181

166182
SizeTy size() const {
167-
switch (Ty) {
168-
case UINT32:
183+
if (std::holds_alternative<uint32_t>(Val))
169184
return sizeof(uint32_t);
170-
case BYTE_ARRAY:
185+
if (std::holds_alternative<std::byte *>(Val))
171186
return getRawByteArraySize();
172-
default:
173-
llvm_unreachable_internal("unsupported SYCL property type");
174-
}
187+
llvm_unreachable_internal("unsupported SYCL property type");
175188
}
176189

177190
private:
178191
void copy(const SYCLPropertyValue &P);
179192

180-
Type Ty = NONE;
181193
std::variant<uint32_t, std::byte *> Val;
182194
};
183195

@@ -207,21 +219,21 @@ class SYCLPropertySetRegistry {
207219
using MapTy = MapVector<SmallString<16>, SYCLPropertySet, SYCLPropertyMapTy>;
208220

209221
// Specific property category names used by tools.
210-
static constexpr char SYCL_SPECIALIZATION_CONSTANTS[] =
222+
static constexpr char SYCLSpecializationConstants[] =
211223
"SYCL/specialization constants";
212-
static constexpr char SYCL_SPEC_CONSTANTS_DEFAULT_VALUES[] =
224+
static constexpr char SYCLSpecConstantsDefaultValues[] =
213225
"SYCL/specialization constants default values";
214-
static constexpr char SYCL_DEVICELIB_REQ_MASK[] = "SYCL/devicelib req mask";
215-
static constexpr char SYCL_KERNEL_PARAM_OPT_INFO[] = "SYCL/kernel param opt";
216-
static constexpr char SYCL_PROGRAM_METADATA[] = "SYCL/program metadata";
217-
static constexpr char SYCL_MISC_PROP[] = "SYCL/misc properties";
218-
static constexpr char SYCL_ASSERT_USED[] = "SYCL/assert used";
219-
static constexpr char SYCL_EXPORTED_SYMBOLS[] = "SYCL/exported symbols";
220-
static constexpr char SYCL_IMPORTED_SYMBOLS[] = "SYCL/imported symbols";
221-
static constexpr char SYCL_DEVICE_GLOBALS[] = "SYCL/device globals";
222-
static constexpr char SYCL_DEVICE_REQUIREMENTS[] = "SYCL/device requirements";
223-
static constexpr char SYCL_HOST_PIPES[] = "SYCL/host pipes";
224-
static constexpr char SYCL_VIRTUAL_FUNCTIONS[] = "SYCL/virtual functions";
226+
static constexpr char SYCLDeviceLibReqMask[] = "SYCL/devicelib req mask";
227+
static constexpr char SYCLKernelParamOptInfo[] = "SYCL/kernel param opt";
228+
static constexpr char SYCLProgramMetadata[] = "SYCL/program metadata";
229+
static constexpr char SYCLMiscProp[] = "SYCL/misc properties";
230+
static constexpr char SYCLAssertUsed[] = "SYCL/assert used";
231+
static constexpr char SYCLExportedSymbols[] = "SYCL/exported symbols";
232+
static constexpr char SYCLImportedSymbols[] = "SYCL/imported symbols";
233+
static constexpr char SYCLDeviceGlobals[] = "SYCL/device globals";
234+
static constexpr char SYCLDeviceRequirements[] = "SYCL/device requirements";
235+
static constexpr char SYCLHostPipes[] = "SYCL/host pipes";
236+
static constexpr char SYCLVirtualFunctions[] = "SYCL/virtual functions";
225237

226238
/// Function for bulk addition of an entire property set in the given
227239
/// \p Category .

‎llvm/lib/Support/SYCLPropertySetIO.cpp‎

Lines changed: 35 additions & 74 deletions
Original file line numberDiff line numberDiff line change
@@ -20,14 +20,10 @@
2020
using namespace llvm::util;
2121
using namespace llvm;
2222

23-
namespace {
24-
25-
::llvm::Error makeError(const Twine &Msg) {
23+
static llvm::Error makeError(const Twine &Msg) {
2624
return createStringError(std::error_code{}, Msg);
2725
}
2826

29-
} // anonymous namespace
30-
3127
Expected<std::unique_ptr<SYCLPropertySetRegistry>>
3228
SYCLPropertySetRegistry::read(const MemoryBuffer *Buf) {
3329
auto Res = std::make_unique<SYCLPropertySetRegistry>();
@@ -37,7 +33,7 @@ SYCLPropertySetRegistry::read(const MemoryBuffer *Buf) {
3733
// See if this line starts a new property set
3834
if (LI->starts_with("[")) {
3935
// Parse the category (property name)
40-
auto EndPos = LI->rfind(']');
36+
size_t EndPos = LI->rfind(']');
4137
if (EndPos == StringRef::npos)
4238
return makeError("invalid line: " + *LI);
4339
StringRef Category = LI->substr(1, EndPos - 1);
@@ -47,49 +43,46 @@ SYCLPropertySetRegistry::read(const MemoryBuffer *Buf) {
4743
if (!CurPropSet)
4844
return makeError("property category missing");
4945
// Parse name and type+value
50-
auto Parts = LI->split('=');
46+
auto [PropName, PropTypeAndValue] = LI->split('=');
5147

52-
if (Parts.first.empty() || Parts.second.empty())
48+
if (PropName.empty() || PropTypeAndValue.empty())
5349
return makeError("invalid property line: " + *LI);
54-
auto TypeVal = Parts.second.split('|');
50+
auto [PropType, PropVal] = PropTypeAndValue.split('|');
5551

56-
if (TypeVal.first.empty() || TypeVal.second.empty())
57-
return makeError("invalid property value: " + Parts.second);
52+
if (PropType.empty() || PropVal.empty())
53+
return makeError("invalid property value: " + PropTypeAndValue);
5854
APInt Tint;
5955

6056
// Parse type
61-
if (TypeVal.first.getAsInteger(10, Tint))
62-
return makeError("invalid property type: " + TypeVal.first);
57+
if (PropType.getAsInteger(10, Tint))
58+
return makeError("invalid property type: " + PropType);
6359
Expected<SYCLPropertyValue::Type> Ttag =
6460
SYCLPropertyValue::getTypeTag(static_cast<int>(Tint.getSExtValue()));
65-
StringRef Val = TypeVal.second;
61+
StringRef Val = PropVal;
6662

6763
if (!Ttag)
6864
return Ttag.takeError();
6965
SYCLPropertyValue Prop(Ttag.get());
7066

7167
// Parse value depending on its type
72-
switch (Ttag.get()) {
73-
case SYCLPropertyValue::Type::UINT32: {
68+
if (Prop.getType() == SYCLPropertyValue::Type::UInt32) {
7469
APInt ValV;
7570
if (Val.getAsInteger(10, ValV))
7671
return createStringError(std::error_code{},
7772
"invalid property value: ", Val.data());
7873
Prop.set(static_cast<uint32_t>(ValV.getZExtValue()));
79-
break;
80-
}
81-
case SYCLPropertyValue::Type::BYTE_ARRAY: {
74+
} else if (Prop.getType() == SYCLPropertyValue::Type::ByteArray) {
8275
std::vector<char> Output;
76+
// Output resized to maximum output size for base64 decoding
77+
Output.resize(((Val.size() + 3) / 4) * 3);
8378
if (Error Err = decodeBase64(Val, Output))
8479
return std::move(Err);
8580
Prop.set(reinterpret_cast<std::byte *>(Output.data()), Output.size());
86-
break;
87-
}
88-
default:
81+
} else {
8982
return createStringError(std::error_code{},
90-
"unsupported property type: ", Ttag.get());
83+
"unsupported property type\n");
9184
}
92-
(*CurPropSet)[Parts.first] = std::move(Prop);
85+
(*CurPropSet)[PropName] = std::move(Prop);
9386
}
9487
if (!CurPropSet)
9588
return makeError("invalid property set registry");
@@ -101,50 +94,36 @@ namespace llvm {
10194
// Output a property to a stream
10295
raw_ostream &operator<<(raw_ostream &Out, const SYCLPropertyValue &Prop) {
10396
Out << static_cast<int>(Prop.getType()) << '|';
104-
switch (Prop.getType()) {
105-
case SYCLPropertyValue::Type::UINT32:
97+
if (Prop.getType() == SYCLPropertyValue::Type::UInt32) {
10698
Out << Prop.asUint32();
107-
break;
108-
case SYCLPropertyValue::Type::BYTE_ARRAY: {
109-
auto PropArr = Prop.asByteArray();
99+
return Out;
100+
}
101+
if (Prop.getType() == SYCLPropertyValue::Type::ByteArray) {
102+
const std::byte *PropArr = Prop.asByteArray();
110103
std::vector<std::byte> V(PropArr, PropArr + Prop.getByteArraySize() /
111104
sizeof(std::byte));
112105
Out << encodeBase64(V);
113-
break;
114-
}
115-
default:
116-
llvm_unreachable(
117-
("unsupported property type: " + utostr(Prop.getType())).c_str());
106+
return Out;
118107
}
119-
return Out;
108+
llvm_unreachable("unsupported property type");
120109
}
121110
} // namespace llvm
122111

123112
void SYCLPropertySetRegistry::write(raw_ostream &Out) const {
124-
for (const auto &PropSet : PropSetMap) {
125-
Out << '[' << PropSet.first << "]\n";
113+
for (const auto &[PropCategory, Props] : PropSetMap) {
114+
Out << '[' << PropCategory << "]\n";
126115

127-
for (const auto &Props : PropSet.second)
128-
Out << Props.first << '=' << Props.second << '\n';
116+
for (const auto &[PropName, PropVal] : Props)
117+
Out << PropName << '=' << PropVal << '\n';
129118
}
130119
}
131120

132121
namespace llvm {
133122
namespace util {
134123

135-
template <> SYCLPropertyValue::Type SYCLPropertyValue::getTypeTag<uint32_t>() {
136-
return UINT32;
137-
}
138-
139-
template <>
140-
SYCLPropertyValue::Type SYCLPropertyValue::getTypeTag<std::byte *>() {
141-
return BYTE_ARRAY;
142-
}
143-
144-
SYCLPropertyValue::SYCLPropertyValue(const std::byte *Data, SizeTy DataBitSize)
145-
: Ty(BYTE_ARRAY) {
146-
constexpr int ByteSizeInBits = 8;
147-
SizeTy DataSize = (DataBitSize + (ByteSizeInBits - 1)) / ByteSizeInBits;
124+
SYCLPropertyValue::SYCLPropertyValue(const std::byte *Data,
125+
SizeTy DataBitSize) {
126+
SizeTy DataSize = (DataBitSize + (CHAR_BIT - 1)) / CHAR_BIT;
148127
constexpr size_t SizeFieldSize = sizeof(SizeTy);
149128

150129
// Allocate space for size and data.
@@ -154,7 +133,7 @@ SYCLPropertyValue::SYCLPropertyValue(const std::byte *Data, SizeTy DataBitSize)
154133
for (size_t I = 0; I < SizeFieldSize; ++I) {
155134
auto ByteArrayVal = std::get<std::byte *>(Val);
156135
ByteArrayVal[I] = (std::byte)DataBitSize;
157-
DataBitSize >>= ByteSizeInBits;
136+
DataBitSize >>= CHAR_BIT;
158137
}
159138
// Append data.
160139
auto ByteArrayVal = std::get<std::byte *>(Val);
@@ -170,38 +149,20 @@ SYCLPropertyValue::SYCLPropertyValue(SYCLPropertyValue &&P) {
170149
SYCLPropertyValue &SYCLPropertyValue::operator=(SYCLPropertyValue &&P) {
171150
copy(P);
172151

173-
if (P.getType() == BYTE_ARRAY)
152+
if (std::holds_alternative<std::byte *>(Val))
174153
P.Val = nullptr;
175-
P.Ty = NONE;
176154
return *this;
177155
}
178156

179157
SYCLPropertyValue &SYCLPropertyValue::operator=(const SYCLPropertyValue &P) {
180-
if (P.getType() == BYTE_ARRAY)
158+
if (std::holds_alternative<std::byte *>(Val))
181159
*this = SYCLPropertyValue(P.asByteArray(), P.getByteArraySizeInBits());
182160
else
183161
copy(P);
184162
return *this;
185163
}
186164

187-
void SYCLPropertyValue::copy(const SYCLPropertyValue &P) {
188-
Ty = P.Ty;
189-
Val = P.Val;
190-
}
191-
192-
constexpr char SYCLPropertySetRegistry::SYCL_SPECIALIZATION_CONSTANTS[];
193-
constexpr char SYCLPropertySetRegistry::SYCL_DEVICELIB_REQ_MASK[];
194-
constexpr char SYCLPropertySetRegistry::SYCL_SPEC_CONSTANTS_DEFAULT_VALUES[];
195-
constexpr char SYCLPropertySetRegistry::SYCL_KERNEL_PARAM_OPT_INFO[];
196-
constexpr char SYCLPropertySetRegistry::SYCL_PROGRAM_METADATA[];
197-
constexpr char SYCLPropertySetRegistry::SYCL_MISC_PROP[];
198-
constexpr char SYCLPropertySetRegistry::SYCL_ASSERT_USED[];
199-
constexpr char SYCLPropertySetRegistry::SYCL_EXPORTED_SYMBOLS[];
200-
constexpr char SYCLPropertySetRegistry::SYCL_IMPORTED_SYMBOLS[];
201-
constexpr char SYCLPropertySetRegistry::SYCL_DEVICE_GLOBALS[];
202-
constexpr char SYCLPropertySetRegistry::SYCL_DEVICE_REQUIREMENTS[];
203-
constexpr char SYCLPropertySetRegistry::SYCL_HOST_PIPES[];
204-
constexpr char SYCLPropertySetRegistry::SYCL_VIRTUAL_FUNCTIONS[];
165+
void SYCLPropertyValue::copy(const SYCLPropertyValue &P) { Val = P.Val; }
205166

206167
} // namespace util
207168
} // namespace llvm

‎llvm/unittests/Support/SYCLPropertySetIOTest.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ TEST(SYCLPropertySet, IncorrectValuesIO) {
5959
<< "Invalid property type";
6060

6161
Content = "[Staff/Ages]\n"
62-
"person1=2|10\n";
62+
"person1=1|IAQ\n";
6363
MemBuf = MemoryBuffer::getMemBuffer(Content);
6464
// Parse a property set registry
6565
PropSetsPtr = SYCLPropertySetRegistry::read(MemBuf.get());

0 commit comments

Comments
 (0)