Skip to content

Commit d0d6ca9

Browse files
adazem009adazem009
committed
Don't optimize numeric string constants used for costumes/sounds
1 parent 378ff28 commit d0d6ca9

12 files changed

+213
-16
lines changed

‎src/engine/internal/llvm/llvmbuildutils.cpp

Lines changed: 35 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55
#include <scratchcpp/list.h>
66
#include <scratchcpp/blockprototype.h>
77
#include <scratchcpp/compiler.h>
8+
#include <scratchcpp/iengine.h>
9+
#include <scratchcpp/costume.h>
810

911
#include "llvmbuildutils.h"
1012
#include "llvmfunctions.h"
@@ -39,6 +41,35 @@ LLVMBuildUtils::LLVMBuildUtils(LLVMCompilerContext *ctx, llvm::IRBuilder<> &buil
3941
initTypes();
4042
createVariableMap();
4143
createListMap();
44+
45+
// Find unsafe numeric string constants in costume and sound names
46+
if (m_target) {
47+
IEngine *engine = m_target->engine();
48+
49+
if (engine) {
50+
auto checkConstant = [this](const std::string &str) {
51+
Value stringValue(str);
52+
Value numberValue(stringValue.toDouble());
53+
54+
if (stringValue.isValidNumber() && str == numberValue.toString())
55+
m_unsafeConstants.insert(str);
56+
};
57+
58+
const auto &targets = engine->targets();
59+
bool found = false;
60+
61+
for (const auto &target : targets) {
62+
const auto &costumes = target->costumes();
63+
const auto &sounds = target->sounds();
64+
65+
for (const auto &costume : costumes)
66+
checkConstant(costume->name());
67+
68+
for (const auto &sound : sounds)
69+
checkConstant(sound->name());
70+
}
71+
}
72+
}
4273
}
4374

4475
void LLVMBuildUtils::init(llvm::Function *function, BlockPrototype *procedurePrototype, bool warp, const std::vector<std::shared_ptr<LLVMRegister>> &regs)
@@ -429,18 +460,18 @@ std::vector<LLVMLoop> &LLVMBuildUtils::loops()
429460
return m_loops;
430461
}
431462

432-
Compiler::StaticType LLVMBuildUtils::optimizeRegisterType(const LLVMRegister *reg)
463+
Compiler::StaticType LLVMBuildUtils::optimizeRegisterType(const LLVMRegister *reg) const
433464
{
434465
Compiler::StaticType ret = reg->type();
435466

436467
// Optimize string constants that represent numbers
437468
if (reg->isConst() && reg->type() == Compiler::StaticType::String) {
438469
const Value &value = reg->constValue();
439470
Value numberValue(value.toDouble());
471+
std::string str = value.toString();
440472

441-
// Apply this optimization only if the number matches the string
442-
// TODO: Exclude unsafe constants
443-
if (value.isValidNumber() && numberValue.toString() == value.toString())
473+
// Apply this optimization only if the number matches the string and the constant is safe
474+
if (value.isValidNumber() && numberValue.toString() == str && m_unsafeConstants.find(str) == m_unsafeConstants.cend())
444475
ret = Compiler::StaticType::Number;
445476
}
446477

‎src/engine/internal/llvm/llvmbuildutils.h

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,8 @@ class LLVMBuildUtils
8383
std::vector<LLVMIfStatement> &ifStatements();
8484
std::vector<LLVMLoop> &loops();
8585

86-
static Compiler::StaticType optimizeRegisterType(const LLVMRegister *reg);
86+
Compiler::StaticType optimizeRegisterType(const LLVMRegister *reg) const;
87+
8788
static Compiler::StaticType mapType(ValueType type);
8889
static ValueType mapType(Compiler::StaticType type);
8990
static bool isSingleType(Compiler::StaticType type);
@@ -161,6 +162,8 @@ class LLVMBuildUtils
161162
bool m_warp = false;
162163
Compiler::CodeType m_codeType = Compiler::CodeType::Script;
163164

165+
std::unordered_set<std::string> m_unsafeConstants;
166+
164167
llvm::Value *m_executionContextPtr = nullptr;
165168
llvm::Value *m_targetPtr = nullptr;
166169
llvm::Value *m_targetVariables = nullptr;

‎src/engine/internal/llvm/llvmcodeanalyzer.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -294,5 +294,5 @@ Compiler::StaticType LLVMCodeAnalyzer::writeType(LLVMInstruction *ins) const
294294
}
295295
}
296296

297-
return LLVMBuildUtils::optimizeRegisterType(argReg);
297+
return m_utils.optimizeRegisterType(argReg);
298298
}

‎test/blocks/control_blocks_test.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ using namespace libscratchcpp;
2323
using namespace libscratchcpp::test;
2424

2525
using ::testing::Return;
26+
using ::testing::ReturnRef;
2627
using ::testing::SaveArg;
2728
using ::testing::_;
2829

@@ -35,6 +36,8 @@ class ControlBlocksTest : public testing::Test
3536
m_engine = m_project.engine().get();
3637
m_extension->registerBlocks(m_engine);
3738
registerBlocks(m_engine, m_extension.get());
39+
40+
EXPECT_CALL(m_engineMock, targets()).WillRepeatedly(ReturnRef(m_engine->targets()));
3841
}
3942

4043
std::unique_ptr<IExtension> m_extension;

‎test/blocks/event_blocks_test.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ using namespace libscratchcpp;
2020
using namespace libscratchcpp::test;
2121

2222
using ::testing::Return;
23+
using ::testing::ReturnRef;
2324

2425
class EventBlocksTest : public testing::Test
2526
{
@@ -29,6 +30,8 @@ class EventBlocksTest : public testing::Test
2930
m_extension = std::make_unique<EventBlocks>();
3031
m_engine = m_project.engine().get();
3132
m_extension->registerBlocks(m_engine);
33+
34+
EXPECT_CALL(m_engineMock, targets()).WillRepeatedly(ReturnRef(m_engine->targets()));
3235
}
3336

3437
std::unique_ptr<IExtension> m_extension;

‎test/blocks/looks_blocks_test.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ using namespace libscratchcpp;
2323
using namespace libscratchcpp::test;
2424

2525
using ::testing::Return;
26+
using ::testing::ReturnRef;
2627
using ::testing::ReturnArg;
2728
using ::testing::_;
2829

@@ -36,6 +37,8 @@ class LooksBlocksTest : public testing::Test
3637
m_extension->registerBlocks(m_engine);
3738
m_extension->onInit(m_engine);
3839

40+
EXPECT_CALL(m_engineMock, targets()).WillRepeatedly(ReturnRef(m_engine->targets()));
41+
3942
// Create and register fake graphic effects
4043
auto colorEffect = std::make_shared<GraphicsEffectMock>();
4144
auto fisheyeEffect = std::make_shared<GraphicsEffectMock>();

‎test/blocks/motion_blocks_test.cpp

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ using namespace libscratchcpp;
2020
using namespace libscratchcpp::test;
2121

2222
using ::testing::Return;
23+
using ::testing::ReturnRef;
2324

2425
class MotionBlocksTest : public testing::Test
2526
{
@@ -29,6 +30,8 @@ class MotionBlocksTest : public testing::Test
2930
m_extension = std::make_unique<MotionBlocks>();
3031
m_engine = m_project.engine().get();
3132
m_extension->registerBlocks(m_engine);
33+
34+
EXPECT_CALL(m_engineMock, targets()).WillRepeatedly(ReturnRef(m_engine->targets()));
3235
}
3336

3437
std::unique_ptr<IExtension> m_extension;

‎test/blocks/sensing_blocks_test.cpp

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,8 @@ class SensingBlocksTest : public testing::Test
4747
EXPECT_CALL(m_audioInput, audioLoudness()).WillRepeatedly(Return(m_audioLoudness));
4848

4949
SensingBlocks::clock = &m_clock;
50+
51+
EXPECT_CALL(m_engineMock, targets()).WillRepeatedly(ReturnRef(m_engine->targets()));
5052
}
5153

5254
void TearDown() override

‎test/llvm/code_analyzer/list_type_analysis.cpp

Lines changed: 82 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
#include <scratchcpp/project.h>
2-
#include <scratchcpp/target.h>
2+
#include <scratchcpp/sprite.h>
3+
#include <scratchcpp/costume.h>
4+
#include <scratchcpp/sound.h>
5+
#include <scratchcpp/iengine.h>
36
#include <scratchcpp/list.h>
47
#include <engine/internal/llvm/llvmcodeanalyzer.h>
58
#include <engine/internal/llvm/llvmcompilercontext.h>
@@ -18,17 +21,33 @@ class LLVMCodeAnalyzer_ListTypeAnalysis : public testing::Test
1821
void SetUp() override
1922
{
2023
auto engine = m_project.engine();
21-
m_ctx = std::make_unique<LLVMCompilerContext>(engine.get(), &m_target);
24+
m_target = std::make_shared<Target>();
25+
m_spriteWithUnsafeConstants = std::make_shared<Sprite>();
26+
27+
auto costume = std::make_shared<Costume>(m_unsafeCostumeNumConstant, "", "");
28+
m_spriteWithUnsafeConstants->addCostume(costume);
29+
30+
auto sound = std::make_shared<Sound>(m_unsafeSoundNumConstant, "", "");
31+
m_spriteWithUnsafeConstants->addSound(sound);
32+
33+
engine->setTargets({ m_target, m_spriteWithUnsafeConstants });
34+
35+
m_ctx = std::make_unique<LLVMCompilerContext>(engine.get(), m_target.get());
2236
m_builder = std::make_unique<llvm::IRBuilder<>>(*m_ctx->llvmCtx());
2337
m_utils = std::make_unique<LLVMBuildUtils>(m_ctx.get(), *m_builder, Compiler::CodeType::Script);
2438
m_analyzer = std::make_unique<LLVMCodeAnalyzer>(*m_utils);
2539
}
2640

2741
std::unique_ptr<LLVMCodeAnalyzer> m_analyzer;
2842

43+
const std::string m_safeNumConstant = "3.14";
44+
const std::string m_unsafeCostumeNumConstant = "12";
45+
const std::string m_unsafeSoundNumConstant = "-27.672";
46+
2947
private:
3048
Project m_project;
31-
Target m_target;
49+
std::shared_ptr<Target> m_target;
50+
std::shared_ptr<Sprite> m_spriteWithUnsafeConstants;
3251
std::unique_ptr<LLVMCompilerContext> m_ctx;
3352
std::unique_ptr<llvm::IRBuilder<>> m_builder;
3453
std::unique_ptr<LLVMBuildUtils> m_utils;
@@ -155,7 +174,7 @@ TEST_F(LLVMCodeAnalyzer_ListTypeAnalysis, StringOptimization_AfterClear)
155174
list.addInstruction(clearList);
156175

157176
auto appendList1 = std::make_shared<LLVMInstruction>(LLVMInstruction::Type::AppendToList, false);
158-
LLVMConstantRegister value1(Compiler::StaticType::String, "3.14");
177+
LLVMConstantRegister value1(Compiler::StaticType::String, m_safeNumConstant);
159178
appendList1->targetList = &targetList;
160179
appendList1->args.push_back({ Compiler::StaticType::Unknown, &value1 });
161180
list.addInstruction(appendList1);
@@ -170,7 +189,7 @@ TEST_F(LLVMCodeAnalyzer_ListTypeAnalysis, StringOptimization_AfterClear)
170189

171190
ASSERT_EQ(appendList1->targetType, Compiler::StaticType::Void);
172191

173-
// String "3.14" optimized to Number, so second write sees Number type
192+
// String gets optimized to Number, so second write sees Number type
174193
ASSERT_EQ(appendList2->targetType, Compiler::StaticType::Number);
175194
}
176195

@@ -203,6 +222,64 @@ TEST_F(LLVMCodeAnalyzer_ListTypeAnalysis, StringOptimization_AfterClear_Differen
203222
ASSERT_EQ(appendList2->targetType, Compiler::StaticType::String);
204223
}
205224

225+
TEST_F(LLVMCodeAnalyzer_ListTypeAnalysis, StringOptimization_AfterClear_UnsafeCostumeConstant)
226+
{
227+
LLVMInstructionList list;
228+
List targetList("", "");
229+
230+
auto clearList = std::make_shared<LLVMInstruction>(LLVMInstruction::Type::ClearList, false);
231+
clearList->targetList = &targetList;
232+
list.addInstruction(clearList);
233+
234+
auto appendList1 = std::make_shared<LLVMInstruction>(LLVMInstruction::Type::AppendToList, false);
235+
LLVMConstantRegister value1(Compiler::StaticType::String, m_unsafeCostumeNumConstant);
236+
appendList1->targetList = &targetList;
237+
appendList1->args.push_back({ Compiler::StaticType::Unknown, &value1 });
238+
list.addInstruction(appendList1);
239+
240+
auto appendList2 = std::make_shared<LLVMInstruction>(LLVMInstruction::Type::AppendToList, false);
241+
LLVMConstantRegister value2(Compiler::StaticType::Bool, true);
242+
appendList2->targetList = &targetList;
243+
appendList2->args.push_back({ Compiler::StaticType::Unknown, &value2 });
244+
list.addInstruction(appendList2);
245+
246+
m_analyzer->analyzeScript(list);
247+
248+
ASSERT_EQ(appendList1->targetType, Compiler::StaticType::Void);
249+
250+
// String does NOT get optimized to Number because there's a costume with the same name
251+
ASSERT_EQ(appendList2->targetType, Compiler::StaticType::String);
252+
}
253+
254+
TEST_F(LLVMCodeAnalyzer_ListTypeAnalysis, StringOptimization_AfterClear_UnsafeSoundConstant)
255+
{
256+
LLVMInstructionList list;
257+
List targetList("", "");
258+
259+
auto clearList = std::make_shared<LLVMInstruction>(LLVMInstruction::Type::ClearList, false);
260+
clearList->targetList = &targetList;
261+
list.addInstruction(clearList);
262+
263+
auto appendList1 = std::make_shared<LLVMInstruction>(LLVMInstruction::Type::AppendToList, false);
264+
LLVMConstantRegister value1(Compiler::StaticType::String, m_unsafeSoundNumConstant);
265+
appendList1->targetList = &targetList;
266+
appendList1->args.push_back({ Compiler::StaticType::Unknown, &value1 });
267+
list.addInstruction(appendList1);
268+
269+
auto appendList2 = std::make_shared<LLVMInstruction>(LLVMInstruction::Type::AppendToList, false);
270+
LLVMConstantRegister value2(Compiler::StaticType::Bool, true);
271+
appendList2->targetList = &targetList;
272+
appendList2->args.push_back({ Compiler::StaticType::Unknown, &value2 });
273+
list.addInstruction(appendList2);
274+
275+
m_analyzer->analyzeScript(list);
276+
277+
ASSERT_EQ(appendList1->targetType, Compiler::StaticType::Void);
278+
279+
// String does NOT get optimized to Number because there's a sound with the same name
280+
ASSERT_EQ(appendList2->targetType, Compiler::StaticType::String);
281+
}
282+
206283
TEST_F(LLVMCodeAnalyzer_ListTypeAnalysis, ClearListOperation)
207284
{
208285
LLVMInstructionList list;

0 commit comments

Comments
 (0)