From 2d168cf4d890eec32e22cd0864b3aefbe153179a Mon Sep 17 00:00:00 2001 From: Guilherme Lampert Date: Mon, 19 Oct 2026 22:53:24 +1100 Subject: [PATCH 1/2] Check that instruction immediates fit their encoding dvp-as keeps only the low bits of an immediate that doesn't fit the instruction's field, without a diagnostic: `iaddi VI03, VI10, -18` assembles to the same word as `+14`, since IADDI's immediate is 5 bits, signed. openvcl passed such values through from the source, and its scheduler could create out-of-range offsets of its own when it shifted a load or store across a pointer update. Each instruction's immediate is now checked when it is parsed, using the value openvcl emits (its own evaluation of the expression, or the integer literal dvp-as reads): - iaddi outside -16..15 is emitted as isubiu/iaddiu of the magnitude, the way SCE vcl 1.4beta7 emits every iaddi (`iaddi a, b, -18` becomes `isubiu a, b, 18`). Outside -32767..32767 it is an error. - iaddiu/isubiu outside 0..32767, fcand/fceq/fcor/fcset beyond 24 bits and fsand/fseq/fsor/fsset beyond 12 bits are errors. vcl 1.4beta7 only warns on iaddiu/isubiu and passes the fc* masks through, and dvp-as then truncates them. - lq/sq/ilw/isw offsets outside -1024..1023 are a warning. vcl 1.4beta7 accepts them, and since VU1 addresses wrap at 1024 quadwords the truncated offset reaches the same address. The scheduler no longer makes a change that would push an offset out of the field: moving a store into a branch delay slot, advancing a pointer update above its stores, software-pipeline prefetches and drained stores (treated like an unknown next-iteration offset), and multi-Q cyclic prefixes. Merging two iaddiu now keeps to IADDIU's unsigned range. test/repro/quad_adc_bug.vcl used `iaddiu vi05, vi00, 0xffff`, which assembles as 0x7fff; it now says 0x7fff. Co-Authored-By: Claude Opus 5.5 --- src/CodeGenerator.cpp | 2 + src/Tokenizer.cpp | 64 ++++ src/Tokenizer.h | 1 + src/VuInstructionInfo.cpp | 49 +++ src/VuInstructionInfo.h | 6 + src/VuSchedulerAnalysis.cpp | 55 ++- src/VuSchedulingRules.cpp | 3 +- src/VuTokenResourceAccess.cpp | 88 +++++ src/VuTokenResourceAccess.h | 7 + test/CMakeLists.txt | 1 + test/integration/test_immediate_ranges.cpp | 408 +++++++++++++++++++++ test/repro/quad_adc_bug.vcl | 2 +- 12 files changed, 680 insertions(+), 6 deletions(-) create mode 100644 test/integration/test_immediate_ranges.cpp diff --git a/src/CodeGenerator.cpp b/src/CodeGenerator.cpp index 699abe7..0cff638 100644 --- a/src/CodeGenerator.cpp +++ b/src/CodeGenerator.cpp @@ -840,6 +840,8 @@ namespace long offset = 0; if( !evaluateIntegerExpression( i->immediate(), offset ) ) return false; + if( offset + delta < -1024 || offset + delta > 1023 ) + return false; std::stringstream s; s << (offset + delta); diff --git a/src/Tokenizer.cpp b/src/Tokenizer.cpp index c906fd6..a4135d9 100644 --- a/src/Tokenizer.cpp +++ b/src/Tokenizer.cpp @@ -10,6 +10,7 @@ #include "Tokenizer.h" #include "Error.h" +#include "VuTokenResourceAccess.h" #include "stlhelp.h" #include @@ -371,6 +372,8 @@ bool Tokenizer::handleToken( Token& token ) return false; } + checkImmediate( token ); + // raw VSM mode if( vsmMode() ) @@ -523,6 +526,67 @@ bool Tokenizer::handleToken( Token& token ) /////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// +void Tokenizer::checkImmediate( Token& token ) +{ + // dvp-as keeps only the low bits of an immediate that doesn't fit its + // instruction's field, without a diagnostic. The token is kept either way, + // so a bad immediate doesn't cascade into errors about its destination. + + double value; + long minimum, maximum; + const Token::Argument* immediate = findVuTokenImmediate( token, value, minimum, maximum ); + if( !immediate || ((value >= minimum) && (value <= maximum)) ) + return; + + // an iaddi beyond its 5 bits becomes an isubiu or iaddiu (SCE VCL emits + // every iaddi that way) + + if( VU_OP_IADDI == vuInstructionOpcodeForMnemonic( token.operand()->name() ) ) + { + minimum = -0x7fff; + maximum = 0x7fff; + + if( (value >= minimum) && (value <= maximum) ) + { + const char* name = (value < 0) ? "isubiu" : "iaddiu"; + for( std::list::const_iterator i = m_operands->begin(); i != m_operands->end(); ++i ) + { + if( casecompare( (*i).name(), name ) ) + continue; + + std::stringstream s; + s << static_cast( (value < 0) ? -value : value ); + token.arguments().back().setImmediate( s.str() ); + token.setOperand( &(*i) ); + return; + } + } + } + + std::stringstream message; + if( (value > -1e15) && (value < 1e15) ) + message << static_cast( value ); + else + message << value; + message << " is outside " << minimum << ".." << maximum; + + // an offset only wraps: VU1 data memory is 1024 quadwords and addresses + // wrap, so on VU1 the encoded offset reaches the same address + + if( (immediate->flags() & Token::Argument::INDIRECT) && (value > -0x80000000LL) && (value < 0x80000000LL) ) + { + const long long range = maximum - minimum + 1; + const long long wrapped = (((static_cast( value ) - minimum) % range + range) % range) + minimum; + message << " and assembles as " << wrapped; + Error::Display( Error( "Offset " + message.str(), token, *immediate, true ) ); + return; + } + + Error::Display( Error( "Immediate " + message.str(), token, *immediate ) ); +} + +/////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// + const Operand* Tokenizer::identifyToken( Token& token ) { assert( m_operands ); diff --git a/src/Tokenizer.h b/src/Tokenizer.h index 1269002..f8fc59d 100644 --- a/src/Tokenizer.h +++ b/src/Tokenizer.h @@ -76,6 +76,7 @@ class Tokenizer const Operand* identifyToken( Token& token ); bool handleToken( Token& token ); + void checkImmediate( Token& token ); void setAvailableFloats( unsigned int floats ); void setAvailableIntegers( unsigned int ints ); diff --git a/src/VuInstructionInfo.cpp b/src/VuInstructionInfo.cpp index 6f36f6d..ffea2af 100644 --- a/src/VuInstructionInfo.cpp +++ b/src/VuInstructionInfo.cpp @@ -626,4 +626,53 @@ const char* vuInstructionDescription( const VuInstructionInfo& info ) return "VU instruction."; } +bool vuInstructionImmediateRange( VuInstructionOpcode opcode, long& minimum, long& maximum ) +{ + switch( opcode ) + { + // 5 bits, signed + case VU_OP_IADDI: + minimum = -16; + maximum = 15; + return true; + + // 15 bits, unsigned + case VU_OP_IADDIU: + case VU_OP_ISUBIU: + minimum = 0; + maximum = 0x7fff; + return true; + + // 11 bits, signed + case VU_OP_LQ: + case VU_OP_SQ: + case VU_OP_ILW: + case VU_OP_ISW: + minimum = -1024; + maximum = 1023; + return true; + + // 24 bits + case VU_OP_FCAND: + case VU_OP_FCEQ: + case VU_OP_FCOR: + case VU_OP_FCSET: + minimum = 0; + maximum = 0xffffff; + return true; + + // 12 bits + case VU_OP_FSAND: + case VU_OP_FSEQ: + case VU_OP_FSOR: + case VU_OP_FSSET: + minimum = 0; + maximum = 0xfff; + return true; + + default: + return false; + } +} + } diff --git a/src/VuInstructionInfo.h b/src/VuInstructionInfo.h index 2e3face..ca0844c 100644 --- a/src/VuInstructionInfo.h +++ b/src/VuInstructionInfo.h @@ -239,6 +239,12 @@ std::string normalizeVuMnemonic( const std::string& word ); std::string vuInstructionParameterSummary( const VuInstructionInfo& info ); const char* vuInstructionDescription( const VuInstructionInfo& info ); +// The values an instruction's integer immediate field (the offset, for +// LQ/SQ/ILW/ISW) can encode. dvp-as keeps only the low bits of a value +// outside it, without a diagnostic. Returns false for an instruction +// without such a field. +bool vuInstructionImmediateRange( VuInstructionOpcode opcode, long& minimum, long& maximum ); + } #endif diff --git a/src/VuSchedulerAnalysis.cpp b/src/VuSchedulerAnalysis.cpp index 3260370..4c6b82c 100644 --- a/src/VuSchedulerAnalysis.cpp +++ b/src/VuSchedulerAnalysis.cpp @@ -2398,7 +2398,10 @@ namespace access.hasMemoryBase ? findInductionUpdate( opportunity.inductionUpdates, access.memoryBaseRegister ) : NULL; - if( update && update->stepKnown && access.hasMemoryOffset ) + // an offset the load can't encode is as good as unknown + if( update && update->stepKnown && access.hasMemoryOffset + && access.memoryOffset + update->step >= -1024 + && access.memoryOffset + update->step <= 1023 ) { prefetch.hasNextIterationOffset = true; prefetch.nextIterationOffset = access.memoryOffset + update->step; @@ -2464,8 +2467,14 @@ namespace tokenIndex, stepAfterStore ) ) { - suffixStore.hasNextIterationOffset = true; - suffixStore.nextIterationOffset = access.memoryOffset - stepAfterStore; + // an offset the store can't encode is as good as unknown, which + // blocks the drain (store_without_next_offset) + const long nextIterationOffset = access.memoryOffset - stepAfterStore; + if( nextIterationOffset >= -1024 && nextIterationOffset <= 1023 ) + { + suffixStore.hasNextIterationOffset = true; + suffixStore.nextIterationOffset = nextIterationOffset; + } suffixStore.drainCandidate = suffixStore.usesInductionRegister; } @@ -3396,6 +3405,37 @@ namespace } } + // True when a cyclic-prefix load or store, its offset shifted as above, + // would no longer fit the instruction's 11-bit offset. The insertion point + // is not settled yet when a candidate is checked, so every known update of + // the base register counts. + bool cyclicPrefixOffsetsLeaveImmediateRange( const VuLoopPipelineOpportunity& candidate, + const std::vector& indexedTokens ) + { + for( std::vector::const_iterator p = candidate.multiQCyclicPrefixTokenIndices.begin(); + p != candidate.multiQCyclicPrefixTokenIndices.end(); ++p ) + { + VuTokenResourceAccess access; + if( *p >= indexedTokens.size() + || !buildVuTokenResourceAccess( *indexedTokens[*p], access ) + || access.memoryKind == VU_MEMORY_NONE + || !access.hasMemoryBase + || !access.hasMemoryOffset ) + continue; + + for( std::vector::const_iterator i = candidate.inductionUpdates.begin(); + i != candidate.inductionUpdates.end(); ++i ) + { + if( !i->stepKnown || i->registerName != access.memoryBaseRegister ) + continue; + const long adjustedOffset = access.memoryOffset + i->step; + if( adjustedOffset < -1024 || adjustedOffset > 1023 ) + return true; + } + } + return false; + } + Token adjustedMultiQCyclicPrefixToken( const Token& token, const std::vector& rotations, const std::vector& inductionUpdates, @@ -3550,6 +3590,8 @@ namespace indexedTokens, allowGuardedPlainStores ) ) return false; + if( cyclicPrefixOffsetsLeaveImmediateRange( candidate, indexedTokens ) ) + return false; if( tokenIndicesContainVisibleLabel( candidate.multiQCyclicPrefixTokenIndices, indexedTokens, candidate.labelTokenIndex ) @@ -10368,7 +10410,12 @@ bool advanceVuStoreBaseUpdates( std::list& tokens ) long offset = 0; if( storeUsesBaseWithKnownOffset( *i, induction.registerName, offset ) ) { - (void)offset; + const long adjustedOffset = offset - induction.step; + if( adjustedOffset < -1024 || adjustedOffset > 1023 ) + { + canMove = false; + break; + } continue; } if( tokenReadsOrWritesRegisterKey( *i, induction.registerName ) ) diff --git a/src/VuSchedulingRules.cpp b/src/VuSchedulingRules.cpp index c28ec6d..90fc5c2 100644 --- a/src/VuSchedulingRules.cpp +++ b/src/VuSchedulingRules.cpp @@ -681,8 +681,9 @@ void coalesceAdjacentVuIntegerAdds( std::list& tokens ) continue; } + // IADDIU's immediate is 15 bits, unsigned const long combined = immediate + nextImmediate; - if( combined < -32768 || combined > 32767 || combined == 0 ) + if( combined <= 0 || combined > 32767 ) { ++i; continue; diff --git a/src/VuTokenResourceAccess.cpp b/src/VuTokenResourceAccess.cpp index 0a4b557..b193614 100644 --- a/src/VuTokenResourceAccess.cpp +++ b/src/VuTokenResourceAccess.cpp @@ -5,6 +5,7 @@ #include "Math.h" #include +#include #include namespace vcl @@ -58,6 +59,76 @@ namespace offset = static_cast( e.result() ); return true; } + + // An integer literal as the assembler reads it (hex 0x.., binary 0b.., + // octal 0.., or decimal), optionally signed. + bool parseAssemblerInteger( const std::string& text, double& value ) + { + std::string::size_type i = text.find_first_not_of( " \t" ); + const std::string::size_type end = text.find_last_not_of( " \t" ); + if( std::string::npos == i ) + return false; + + const bool negative = ('-' == text[i]); + if( ('-' == text[i]) || ('+' == text[i]) ) + ++i; + if( i > end ) + return false; + + unsigned int base = 10; + if( ('0' == text[i]) && (i < end) ) + { + const char prefix = text[i+1]; + if( ('x' == prefix) || ('X' == prefix) ) + base = 16; + else if( ('b' == prefix) || ('B' == prefix) ) + base = 2; + else + base = 8; + i += (8 == base) ? 1 : 2; + if( i > end ) + return false; + } + + double result = 0; + for( ; i <= end; ++i ) + { + const char c = text[i]; + unsigned int digit; + if( (c >= '0') && (c <= '9') ) + digit = c - '0'; + else if( (c >= 'a') && (c <= 'f') ) + digit = c - 'a' + 10; + else if( (c >= 'A') && (c <= 'F') ) + digit = c - 'A' + 10; + else + return false; + if( digit >= base ) + return false; + result = (result * base) + digit; + } + + value = negative ? -result : result; + return true; + } + + // The value an immediate is encoded as: CodeGenerator writes what Expression + // can evaluate as an integer (truncated, as in C), and leaves any other text + // to the assembler. + bool evaluateEncodedImmediate( const Token::Argument& arg, double& value ) + { + Expression e; + e.setCustomOperators( Math::mathOperators() ); + if( (arg.flags() & Token::Argument::EVALUATE) && e.process( arg.immediate() ) && e.solve() ) + { + const double result = e.result(); + if( result != result ) // NaN + return false; + value = (result < 0) ? ceil( result ) : floor( result ); + return true; + } + return parseAssemblerInteger( arg.immediate(), value ); + } } VuTokenResourceAccess::VuTokenResourceAccess() @@ -194,4 +265,21 @@ bool buildVuTokenResourceAccess( const Token& token, VuTokenResourceAccess& acce return true; } +const Token::Argument* findVuTokenImmediate( const Token& token, double& value, long& minimum, long& maximum ) +{ + const VuInstructionInfo* info = tokenInstructionInfo( token ); + if( !info || !vuInstructionImmediateRange( info->opcode, minimum, maximum ) ) + return NULL; + + const std::list& args = token.arguments(); + for( std::list::const_iterator i = args.begin(); i != args.end(); ++i ) + { + const bool offset = ((*i).type() == Token::Argument::INTEGER_REGISTER) + && ((*i).flags() & Token::Argument::INDIRECT); + if( ((*i).type() == Token::Argument::IMMEDIATE) || offset ) + return evaluateEncodedImmediate( *i, value ) ? &(*i) : NULL; + } + return NULL; +} + } diff --git a/src/VuTokenResourceAccess.h b/src/VuTokenResourceAccess.h index d84128e..a8eb65d 100644 --- a/src/VuTokenResourceAccess.h +++ b/src/VuTokenResourceAccess.h @@ -40,6 +40,13 @@ bool vuRegisterKey( const Token::Argument& arg, std::string& key ); unsigned int vuReadFieldMask( const Token& token, const Token::Argument& arg ); unsigned int vuWriteFieldMask( const Token& token, const Token::Argument& arg ); +// The argument holding token's integer immediate (or memory offset), the +// value it encodes as and the range its instruction's field can hold (see +// vuInstructionImmediateRange). Returns NULL when the instruction has no +// such field, or when the immediate is not an integer constant (a symbol, +// say), which only the assembler can evaluate. +const Token::Argument* findVuTokenImmediate( const Token& token, double& value, long& minimum, long& maximum ); + } #endif diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index 7e7b7b2..fb0ff41 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -73,6 +73,7 @@ set(OPENVCL_TEST_SOURCES ${CMAKE_CURRENT_SOURCE_DIR}/integration/test_clip_validation.cpp ${CMAKE_CURRENT_SOURCE_DIR}/integration/test_loi_hex.cpp ${CMAKE_CURRENT_SOURCE_DIR}/integration/test_parser_families.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/integration/test_immediate_ranges.cpp ${CMAKE_CURRENT_SOURCE_DIR}/integration/test_instruction_info_dump.cpp ${CMAKE_CURRENT_SOURCE_DIR}/integration/test_pairing.cpp ${CMAKE_CURRENT_SOURCE_DIR}/integration/test_literal_register_conflict.cpp diff --git a/test/integration/test_immediate_ranges.cpp b/test/integration/test_immediate_ranges.cpp new file mode 100644 index 0000000..2d69ca9 --- /dev/null +++ b/test/integration/test_immediate_ranges.cpp @@ -0,0 +1,408 @@ +// Regression tests: an instruction immediate must fit its encoding. +// +// dvp-as keeps only the low bits of an immediate that does not fit the +// instruction's field, and says nothing: 'iaddi VI03, VI10, -18' assembles +// to the same word as '+14', because IADDI's immediate is 5 bits, signed. +// openvcl passed such values through, both from the source and from its own +// offset rewrites, so the first sign of one was on screen. +// +// Now an iaddi outside -16..15 becomes an isubiu or iaddiu (SCE VCL emits +// every iaddi that way), an iaddiu/isubiu or flag-mask immediate that does +// not fit is an error, and a memory offset outside -1024..1023 is a warning +// (the assembler wraps it, which on VU1 reaches the same address). The +// scheduler no longer moves a load or store when that would push its offset +// out of the field. + +#include "test_harness.h" +#include "openvcl_runner.h" + +#include +#include +#include + +namespace +{ + std::string program(const std::string& body) + { + return + "\t.init_vf_all\n" + "\t.init_vi_all\n" + "\t.syntax new\n" + "\t.name vsmImmediateRanges\n" + "\t--enter\n" + "\t--endenter\n" + + body + + "\t--exit\n" + "\t--endexit\n"; + } + + ::test::RunResult compile(const std::string& source) + { + return ::test::run_openvcl(std::vector(), source); + } + + bool contains(const std::string& text, const std::string& needle) + { + return text.find(needle) != std::string::npos; + } + + // True when every "(VIxx)" memory operand in the emitted code + // fits the 11-bit field of lq/sq/ilw/isw. + bool memoryOffsetsFit(const std::string& vsm) + { + for (std::string::size_type i = vsm.find("(VI"); i != std::string::npos; i = vsm.find("(VI", i + 1)) + { + std::string::size_type begin = i; + while (begin > 0 && ((vsm[begin - 1] >= '0' && vsm[begin - 1] <= '9') || vsm[begin - 1] == '-')) + --begin; + if (begin == i) + continue; + const long offset = std::strtol(vsm.substr(begin, i - begin).c_str(), NULL, 10); + if (offset < -1024 || offset > 1023) + return false; + } + return true; + } + + std::string withOffset(const std::string& source, const std::string& offset) + { + std::string result = source; + result.replace(result.find("OFFSET"), 6, offset); + return result; + } + + // A software-pipelined loop that prefetches the next iteration's load, + // lq.xyzw vf01, OFFSET+1(vi01). + const char* kPrefetchLoop = + "\t.init_vf_all\n" + "\t.init_vi_all\n" + "\t--enter\n" + "\t--endenter\n" + "\tiaddiu vi01, vi00, 0\n" + "\tiaddiu vi02, vi00, 3\n" + "\tmove.xyzw vf03, vf00\n" + "loop_lid:\n" + "\t--LoopCS 1,1\n" + "\tlq.xyzw vf01, OFFSET(vi01)\n" + "\tdiv q, vf00w, vf01w\n" + "\tmulq.xyz vf02, vf03, q\n" + "\tadd.xyz vf10, vf00, vf00\n" + "\tadd.xyz vf11, vf00, vf00\n" + "\tadd.xyz vf12, vf00, vf00\n" + "\tadd.xyz vf13, vf00, vf00\n" + "\tadd.xyz vf14, vf00, vf00\n" + "\tadd.xyz vf15, vf00, vf00\n" + "\tiaddiu vi01, vi01, 1\n" + "\tibne vi01, vi02, loop_lid\n" + "\t--exit\n" + "\t--endexit\n"; + + // A software-pipelined loop whose store drains into the next iteration + // at OFFSET-1(vi02). + const char* kSuffixStoreLoop = + "\t.init_vf_all\n" + "\t.init_vi_all\n" + "\t--enter\n" + "\t--endenter\n" + "\tiaddiu vi01, vi00, 0\n" + "\tiaddiu vi02, vi00, 0\n" + "\tiaddiu vi04, vi00, 3\n" + "\tmove.xyzw vf02, vf00\n" + "loop_lid:\n" + "\t--LoopCS 1,1\n" + "\tlq.xyzw vf01, 0(vi01)\n" + "\tdiv q, vf00w, vf01w\n" + "\tmulq.xyz vf03, vf02, q\n" + "\tsq.xyz vf03, OFFSET(vi02)\n" + "\tadd.xyz vf10, vf00, vf00\n" + "\tadd.xyz vf11, vf00, vf00\n" + "\tadd.xyz vf12, vf00, vf00\n" + "\tadd.xyz vf13, vf00, vf00\n" + "\tadd.xyz vf14, vf00, vf00\n" + "\tiaddiu vi01, vi01, 1\n" + "\tiaddiu vi02, vi02, 1\n" + "\tibne vi01, vi04, loop_lid\n" + "\t--exit\n" + "\t--endexit\n"; + + // A multi-Q loop whose cyclic prefix carries lq.xyzw vf03, OFFSET(vi01) + // across the vi01 update. + const char* kMultiQPrefixLoop = + "\t.init_vf_all\n" + "\t.init_vi_all\n" + "\t--enter\n" + "\t--endenter\n" + "\tiaddiu vi01, vi00, 0\n" + "\tiaddiu vi02, vi00, 3\n" + "loop_lid:\n" + "\t--LoopCS 1,1\n" + "\tlq.xyzw vf03, OFFSET(vi01)\n" + "\tdiv q, vf00w, vf03w\n" + "\tmulq.xyz vf02, vf00, q\n" + "\tadd.xyz vf10, vf00, vf00\n" + "\tadd.xyz vf11, vf00, vf00\n" + "\tdiv q, vf00w, vf00w\n" + "\tadd.xyz vf12, vf00, vf00\n" + "\tadd.xyz vf13, vf00, vf00\n" + "\tmulq.xyz vf05, vf00, q\n" + "\tadd.xyz vf14, vf00, vf00\n" + "\tiaddiu vi01, vi01, 1\n" + "\tibne vi01, vi02, loop_lid\n" + "\t--exit\n" + "\t--endexit\n"; + + // A loop whose store can fill the branch delay slot, after the vi01 + // update, as sq.xyzw vf02, OFFSET-1(vi01). + const char* kDelaySlotStoreLoop = + "\t.init_vf_all\n" + "\t.init_vi_all\n" + "\t--enter\n" + "\t--endenter\n" + "\tiaddiu vi01, vi00, 0\n" + "\tiaddiu vi02, vi00, 3\n" + "loop_lid:\n" + "\tlq.xyzw vf01, 0(vi00)\n" + "\tadd.xyzw vf02, vf01, vf01\n" + "\tsq.xyzw vf02, OFFSET(vi01)\n" + "\tiaddiu vi01, vi01, 1\n" + "\tibne vi01, vi02, loop_lid\n" + "\t--exit\n" + "\t--endexit\n"; +} + +TEST_CASE("openvcl: iaddi keeps an immediate that fits its 5 bits") +{ + ::test::RunResult r = compile(program( + "\tiaddi iA, vi00, -16\n" + "\tiaddi iB, vi00, 15\n" + "\tisw.x iA, 0(vi00)\n" + "\tisw.x iB, 1(vi00)\n")); + + CHECK(r.exit_code == 0); + CHECK(r.stderr_data.empty()); + CHECK(contains(r.stdout_data, "iaddi VI01, VI00, -16")); + CHECK(contains(r.stdout_data, "iaddi VI02, VI00, 15")); +} + +TEST_CASE("openvcl: iaddi beyond its 5 bits becomes isubiu or iaddiu") +{ + ::test::RunResult r = compile(program( + "\tilw.x iA, 0(vi00)\n" + "\tiaddi iB, iA, -18\n" + "\tiaddi iC, iA, 16\n" + "\tiaddi iD, iA, (0 - 17)\n" + "\tiaddi iE, iA, -32767\n" + "\tiaddi iF, iA, 32767\n" + "\tisw.x iB, 1(vi00)\n" + "\tisw.x iC, 2(vi00)\n" + "\tisw.x iD, 3(vi00)\n" + "\tisw.x iE, 4(vi00)\n" + "\tisw.x iF, 5(vi00)\n")); + + CHECK(r.exit_code == 0); + CHECK(r.stderr_data.empty()); + CHECK(contains(r.stdout_data, "isubiu VI02, VI01, 18")); + CHECK(contains(r.stdout_data, "iaddiu VI03, VI01, 16")); + CHECK(contains(r.stdout_data, "isubiu VI04, VI01, 17")); + CHECK(contains(r.stdout_data, "isubiu VI05, VI01, 32767")); + CHECK(contains(r.stdout_data, "iaddiu VI06, VI01, 32767")); + CHECK(!contains(r.stdout_data, "iaddi ")); +} + +TEST_CASE("openvcl: iaddi beyond 15 bits is an error") +{ + ::test::RunResult r = compile(program( + "\tiaddi iA, vi00, -32768\n" + "\tisw.x iA, 0(vi00)\n")); + + CHECK(r.exit_code != 0); + CHECK(contains(r.stderr_data, "Immediate -32768 is outside -32767..32767")); +} + +TEST_CASE("openvcl: iaddiu and isubiu take 0..32767") +{ + ::test::RunResult ok = compile(program( + "\tiaddiu iA, vi00, 32767\n" + "\tiaddiu iB, vi00, 0x7fff\n" + "\tisubiu iC, vi00, 0\n" + "\tisw.x iA, 0(vi00)\n" + "\tisw.x iB, 1(vi00)\n" + "\tisw.x iC, 2(vi00)\n")); + CHECK(ok.exit_code == 0); + CHECK(ok.stderr_data.empty()); + + const char* rejected[][2] = + { + { "iaddiu iA, vi00, 32768", "Immediate 32768 is outside 0..32767" }, + { "iaddiu iA, vi00, 0x8000", "Immediate 32768 is outside 0..32767" }, + { "iaddiu iA, vi00, -1", "Immediate -1 is outside 0..32767" }, + { "isubiu iA, vi00, 40000", "Immediate 40000 is outside 0..32767" }, + { "isubiu iA, vi00, -1", "Immediate -1 is outside 0..32767" }, + }; + for (unsigned int i = 0; i < sizeof(rejected) / sizeof(rejected[0]); ++i) + { + ::test::RunResult r = compile(program(std::string("\t") + rejected[i][0] + "\n\tisw.x iA, 0(vi00)\n")); + CHECK(r.exit_code != 0); + CHECK(contains(r.stderr_data, rejected[i][1])); + } +} + +TEST_CASE("openvcl: clip flag masks take 24 bits") +{ + ::test::RunResult ok = compile(program( + "\tfcset 0xffffff\n" + "\tfcand vi01, 0xffffff\n" + "\tisw.x vi01, 0(vi00)\n")); + CHECK(ok.exit_code == 0); + CHECK(ok.stderr_data.empty()); + + const char* rejected[] = + { + "\tfcset 0\n\tfcand vi01, 0x1000000\n\tisw.x vi01, 0(vi00)\n", + "\tfcset 0\n\tfcor vi01, 16777216\n\tisw.x vi01, 0(vi00)\n", + "\tfcset 0\n\tfceq vi01, 0x1000000\n\tisw.x vi01, 0(vi00)\n", + "\tfcset 0x1000000\n\tfcand vi01, 1\n\tisw.x vi01, 0(vi00)\n", + }; + for (unsigned int i = 0; i < sizeof(rejected) / sizeof(rejected[0]); ++i) + { + ::test::RunResult r = compile(program(rejected[i])); + CHECK(r.exit_code != 0); + CHECK(contains(r.stderr_data, "Immediate 16777216 is outside 0..16777215")); + } +} + +TEST_CASE("openvcl: status flag masks take 12 bits") +{ + ::test::RunResult ok = compile(program( + "\tfsset 0xfff\n" + "\tfsand vi01, 0xfff\n" + "\tisw.x vi01, 0(vi00)\n")); + CHECK(ok.exit_code == 0); + CHECK(ok.stderr_data.empty()); + + const char* rejected[] = + { + "\tfsand vi01, 0x1000\n\tisw.x vi01, 0(vi00)\n", + "\tfsor vi01, 4096\n\tisw.x vi01, 0(vi00)\n", + "\tfseq vi01, 0x1000\n\tisw.x vi01, 0(vi00)\n", + "\tfsset 0x1000\n", + }; + for (unsigned int i = 0; i < sizeof(rejected) / sizeof(rejected[0]); ++i) + { + ::test::RunResult r = compile(program(rejected[i])); + CHECK(r.exit_code != 0); + CHECK(contains(r.stderr_data, "Immediate 4096 is outside 0..4095")); + } +} + +TEST_CASE("openvcl: an immediate error does not cascade into later lines") +{ + ::test::RunResult r = compile(program( + "\tiaddiu iA, vi00, 40000\n" + "\tisw.x iA, 0(vi00)\n")); + + CHECK(r.exit_code != 0); + CHECK(contains(r.stderr_data, "Immediate 40000 is outside 0..32767 (iaddiu iA, vi00, >>> 40000 <<<)")); + CHECK(r.stderr_data.find('\n') == r.stderr_data.size() - 1); +} + +TEST_CASE("openvcl: a memory offset outside 11 bits is a warning") +{ + ::test::RunResult edges = compile(program( + "\tlq fA, 1023(vi00)\n" + "\tsq fA, -1024(vi00)\n" + "\tilw.x iA, 1023(vi00)\n" + "\tisw.x iA, -1024(vi00)\n")); + CHECK(edges.exit_code == 0); + CHECK(edges.stderr_data.empty()); + + ::test::RunResult r = compile(program( + "\tlq fA, 1024(vi00)\n" + "\tsq fA, (1010 + 15)(vi00)\n" + "\tilw.x iA, -1025(vi00)\n" + "\tisw.x iA, 2000(vi00)\n" + "\tsq fA, 0x400(vi00)\n")); + CHECK(r.exit_code == 0); + CHECK(contains(r.stderr_data, "warning: Offset 1024 is outside -1024..1023 and assembles as -1024 (lq fA, >>> 1024(vi00) <<<)")); + CHECK(contains(r.stderr_data, "warning: Offset 1025 is outside -1024..1023 and assembles as -1023")); + CHECK(contains(r.stderr_data, "warning: Offset -1025 is outside -1024..1023 and assembles as 1023")); + CHECK(contains(r.stderr_data, "warning: Offset 2000 is outside -1024..1023 and assembles as -48")); + CHECK(contains(r.stderr_data, "warning: Offset 1024 is outside -1024..1023 and assembles as -1024 (sq fA, >>> 0x400(vi00) <<<)")); + CHECK(contains(r.stdout_data, "lq VF01, 1024(VI00)")); + CHECK(contains(r.stdout_data, "isw.x VI01, 2000(VI00)")); +} + +TEST_CASE("openvcl: immediates only the assembler can evaluate are left to it") +{ + ::test::RunResult r = compile(program( + "\tiaddiu iA, vi00, kVertexCount\n" + "\tisw.x iA, 0(vi00)\n" + "\tlq fA, kBase+1(vi00)\n" + "\tsq fA, kBase+2(vi00)\n")); + + CHECK(r.exit_code == 0); + CHECK(r.stderr_data.empty()); + CHECK(contains(r.stdout_data, "iaddiu VI01, VI00, kVertexCount")); + CHECK(contains(r.stdout_data, "lq VF01, kBase+1(VI00)")); +} + +TEST_CASE("openvcl: software pipelining keeps a prefetch offset encodable") +{ + ::test::RunResult fits = compile(withOffset(kPrefetchLoop, "1022")); + CHECK(fits.exit_code == 0); + CHECK(contains(fits.stdout_data, "loop_lid__PROLOG:")); + CHECK(contains(fits.stdout_data, "lq VF01, 1023(VI01)")); + + // the next iteration's lq.xyzw vf01, 1024(vi01) would not fit + ::test::RunResult r = compile(withOffset(kPrefetchLoop, "1023")); + CHECK(r.exit_code == 0); + CHECK(r.stderr_data.empty()); + CHECK(memoryOffsetsFit(r.stdout_data)); +} + +TEST_CASE("openvcl: software pipelining keeps a drained store offset encodable") +{ + ::test::RunResult fits = compile(withOffset(kSuffixStoreLoop, "-1022")); + CHECK(fits.exit_code == 0); + CHECK(contains(fits.stdout_data, "loop_lid__DRAIN:")); + CHECK(contains(fits.stdout_data, "sq.xyz VF03, -1024(VI02)")); + CHECK(memoryOffsetsFit(fits.stdout_data)); + + // draining (and advancing the vi02 update) would need -1025(vi02) or below + const char* offsets[] = { "-1023", "-1024" }; + for (unsigned int i = 0; i < sizeof(offsets) / sizeof(offsets[0]); ++i) + { + ::test::RunResult r = compile(withOffset(kSuffixStoreLoop, offsets[i])); + CHECK(r.exit_code == 0); + CHECK(r.stderr_data.empty()); + CHECK(memoryOffsetsFit(r.stdout_data)); + } +} + +TEST_CASE("openvcl: software pipelining keeps a cyclic prefix offset encodable") +{ + ::test::RunResult fits = compile(withOffset(kMultiQPrefixLoop, "1022")); + CHECK(fits.exit_code == 0); + CHECK(contains(fits.stdout_data, "loop_lid__PROLOG:")); + CHECK(contains(fits.stdout_data, "lq VF03, 1023(VI01)")); + + // the prefix load would become lq.xyzw vf03, 1024(vi01) + ::test::RunResult r = compile(withOffset(kMultiQPrefixLoop, "1023")); + CHECK(r.exit_code == 0); + CHECK(r.stderr_data.empty()); + CHECK(memoryOffsetsFit(r.stdout_data)); +} + +TEST_CASE("openvcl: a store moved into a branch delay slot keeps its offset encodable") +{ + ::test::RunResult moved = compile(withOffset(kDelaySlotStoreLoop, "-1023")); + CHECK(moved.exit_code == 0); + CHECK(contains(moved.stdout_data, "sq VF02, -1024(VI01)")); + + // after the vi01 update the store would need -1025(vi01) + ::test::RunResult r = compile(withOffset(kDelaySlotStoreLoop, "-1024")); + CHECK(r.exit_code == 0); + CHECK(r.stderr_data.empty()); + CHECK(contains(r.stdout_data, "sq VF02, -1024(VI01)")); + CHECK(memoryOffsetsFit(r.stdout_data)); +} diff --git a/test/repro/quad_adc_bug.vcl b/test/repro/quad_adc_bug.vcl index 9f98d8e..3931476 100644 --- a/test/repro/quad_adc_bug.vcl +++ b/test/repro/quad_adc_bug.vcl @@ -30,7 +30,7 @@ main_loop_lid: xtop vi04 - iaddiu vi05, vi00, 0xffff + iaddiu vi05, vi00, 0x7fff lq vf07, 0(vi00) lq vf08, 1(vi00) lq vf09, 2(vi00) From 5b486ff5e57eb3a599776d9206cae779f02462f8 Mon Sep 17 00:00:00 2001 From: Guilherme Lampert Date: Mon, 19 Oct 2026 22:53:55 +1100 Subject: [PATCH 2/2] Add --warnings-as-errors With it, any warning fails the run with a non-zero exit status, as an error does. The first warning openvcl reports is the out-of-range memory offset added in the previous commit, which a build can now choose to stop on. The warning text is unchanged. Co-Authored-By: Claude Opus 5.5 --- src/CommandLine.cpp | 4 ++++ src/CommandLine.h | 3 +++ src/CommandLine.inl | 7 ++++++ src/Error.cpp | 15 +++++++++---- src/Error.h | 5 +++++ src/Parser.cpp | 2 ++ test/integration/test_immediate_ranges.cpp | 26 +++++++++++++++++++--- 7 files changed, 55 insertions(+), 7 deletions(-) diff --git a/src/CommandLine.cpp b/src/CommandLine.cpp index e6a6dea..6b7c70e 100644 --- a/src/CommandLine.cpp +++ b/src/CommandLine.cpp @@ -77,6 +77,7 @@ CommandLine::CommandLine() m_knownLoopOptimizations = false; m_genericSoftwarePipelining = true; m_strictScheduleSlots = false; + m_warningsAsErrors = false; m_gasp = "gasp"; m_cpp = "cpp"; @@ -129,6 +130,7 @@ CommandLine::CommandLine() m_options.push_back(Option('\0',"enable-generic-software-pipelining",ENABLE_GENERIC_SOFTWARE_PIPELINING,false)); m_options.push_back(Option('\0',"disable-generic-software-pipelining",DISABLE_GENERIC_SOFTWARE_PIPELINING,false)); m_options.push_back(Option('\0',"strict-schedule-slots",STRICT_SCHEDULE_SLOTS,false)); + m_options.push_back(Option('\0',"warnings-as-errors",WARNINGS_AS_ERRORS,false)); } /////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// @@ -243,6 +245,7 @@ bool CommandLine::parse( int argc, char* argv[] ) case ENABLE_GENERIC_SOFTWARE_PIPELINING: m_genericSoftwarePipelining = true; break; case DISABLE_GENERIC_SOFTWARE_PIPELINING: m_genericSoftwarePipelining = false; break; case STRICT_SCHEDULE_SLOTS: m_strictScheduleSlots = true; break; + case WARNINGS_AS_ERRORS: m_warningsAsErrors = true; break; case ANALYZE_VSM_COST_LOOP: { std::string::size_type separator = argument.find('='); @@ -334,6 +337,7 @@ void CommandLine::showUsage( std::ostream& stream ) stream << " --enable-generic-software-pipelining Enable generic software-pipeline rewrites. (Default)" << std::endl; stream << " --disable-generic-software-pipelining Disable generic software-pipeline rewrites." << std::endl; stream << " --strict-schedule-slots Emit from scheduler-selected pairs without legacy lookahead fallbacks. (Default for generic codegen)" << std::endl; + stream << " --warnings-as-errors Fail (non-zero exit status) when any warning is reported." << std::endl; stream << std::endl << " If no input or output file are specified, standard I/O will be used instead." << std::endl; diff --git a/src/CommandLine.h b/src/CommandLine.h index f5e4b3d..d7071d7 100644 --- a/src/CommandLine.h +++ b/src/CommandLine.h @@ -58,6 +58,7 @@ class CommandLine bool knownLoopOptimizations() const; bool genericSoftwarePipelining() const; bool strictScheduleSlots() const; + bool warningsAsErrors() const; const std::vector< std::pair >& costLoops() const; const std::string& costCompareBaseline() const; const std::string& costCompareListCheckMetric() const; @@ -134,6 +135,7 @@ class CommandLine ENABLE_GENERIC_SOFTWARE_PIPELINING, DISABLE_GENERIC_SOFTWARE_PIPELINING, STRICT_SCHEDULE_SLOTS, + WARNINGS_AS_ERRORS, IGNORE }; @@ -187,6 +189,7 @@ class CommandLine bool m_knownLoopOptimizations; bool m_genericSoftwarePipelining; bool m_strictScheduleSlots; + bool m_warningsAsErrors; std::vector< std::pair > m_costLoops; std::string m_costCompareBaseline; std::string m_costCompareListCheckMetric; diff --git a/src/CommandLine.inl b/src/CommandLine.inl index 5500026..928a703 100644 --- a/src/CommandLine.inl +++ b/src/CommandLine.inl @@ -189,6 +189,13 @@ inline bool CommandLine::strictScheduleSlots() const /////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// +inline bool CommandLine::warningsAsErrors() const +{ + return m_warningsAsErrors; +} + +/////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// + inline const std::vector< std::pair >& CommandLine::costLoops() const { return m_costLoops; diff --git a/src/Error.cpp b/src/Error.cpp index 3f5cfc3..391d75f 100644 --- a/src/Error.cpp +++ b/src/Error.cpp @@ -8,6 +8,7 @@ namespace vcl std::list Error::ms_errors; unsigned int Error::ms_errorCount = 0; +bool Error::ms_warningsAsErrors = false; /////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// @@ -158,10 +159,11 @@ std::string Error::toString() const void Error::Display( const Error& e ) { std::cerr << e.toString() << std::endl; - // Track non-warning errors so main() can fail the run with a non-zero - // exit code. Without this, openvcl prints diagnostics and still - // reports success, which lets broken VCL silently produce empty .vsm. - if( !e.m_warning ) + // Track non-warning errors (and, with --warnings-as-errors, warnings) so + // main() can fail the run with a non-zero exit code. Without this, + // openvcl prints diagnostics and still reports success, which lets + // broken VCL silently produce empty .vsm. + if( !e.m_warning || ms_warningsAsErrors ) ++ms_errorCount; } @@ -175,4 +177,9 @@ void Error::ResetErrorCount() ms_errorCount = 0; } +void Error::SetWarningsAsErrors( bool enable ) +{ + ms_warningsAsErrors = enable; +} + } diff --git a/src/Error.h b/src/Error.h index a9a3c8e..f098df5 100644 --- a/src/Error.h +++ b/src/Error.h @@ -30,6 +30,10 @@ class Error static bool HasErrors(); static void ResetErrorCount(); + // Count warnings as errors from now on (--warnings-as-errors), so they + // fail the run too. + static void SetWarningsAsErrors( bool enable ); + private: enum @@ -50,6 +54,7 @@ class Error static std::list ms_errors; static unsigned int ms_errorCount; + static bool ms_warningsAsErrors; }; } diff --git a/src/Parser.cpp b/src/Parser.cpp index b8b1946..bdba198 100644 --- a/src/Parser.cpp +++ b/src/Parser.cpp @@ -1159,6 +1159,8 @@ bool Parser::create( int argc, char* argv[] ) return false; } + Error::SetWarningsAsErrors( m_cmdLine.warningsAsErrors() ); + if( m_cmdLine.showVersion() ) setState( SHOW_VERSION ); else if( m_cmdLine.showUsage() ) diff --git a/test/integration/test_immediate_ranges.cpp b/test/integration/test_immediate_ranges.cpp index 2d69ca9..b17982e 100644 --- a/test/integration/test_immediate_ranges.cpp +++ b/test/integration/test_immediate_ranges.cpp @@ -9,9 +9,9 @@ // Now an iaddi outside -16..15 becomes an isubiu or iaddiu (SCE VCL emits // every iaddi that way), an iaddiu/isubiu or flag-mask immediate that does // not fit is an error, and a memory offset outside -1024..1023 is a warning -// (the assembler wraps it, which on VU1 reaches the same address). The -// scheduler no longer moves a load or store when that would push its offset -// out of the field. +// (the assembler wraps it, which on VU1 reaches the same address), which +// --warnings-as-errors turns into a failure. The scheduler no longer moves +// a load or store when that would push its offset out of the field. #include "test_harness.h" #include "openvcl_runner.h" @@ -406,3 +406,23 @@ TEST_CASE("openvcl: a store moved into a branch delay slot keeps its offset enco CHECK(contains(r.stdout_data, "sq VF02, -1024(VI01)")); CHECK(memoryOffsetsFit(r.stdout_data)); } + +TEST_CASE("openvcl: --warnings-as-errors fails the run on a warning") +{ + const std::string warned = program("\tlq fA, 1024(vi00)\n\tsq fA, 0(vi00)\n"); + const std::string clean = program("\tlq fA, 1023(vi00)\n\tsq fA, 0(vi00)\n"); + + ::test::RunResult r = compile(warned); + CHECK(r.exit_code == 0); + CHECK(contains(r.stderr_data, "warning: Offset 1024 is outside -1024..1023")); + + std::vector args; + args.push_back("--warnings-as-errors"); + ::test::RunResult strict = ::test::run_openvcl(args, warned); + CHECK(strict.exit_code != 0); + CHECK(contains(strict.stderr_data, "warning: Offset 1024 is outside -1024..1023")); + + ::test::RunResult strictClean = ::test::run_openvcl(args, clean); + CHECK(strictClean.exit_code == 0); + CHECK(strictClean.stderr_data.empty()); +}