For loop countinng down to 0 doesn't work

[veremenko-y]

The following code is from the examples in the README.md

// x will be 0 after the loop.
for x in 31 .. 0 by -1 {
    // ...
}

Output:

* wiz: version 0.1.2 (alpha)
>> Parsing...
>> Compiling...
src/bugtest.wiz:14: error: inequality comparison `!=` is not defined between provided operand types `u8` and `iexpr`
src/bugtest.wiz:14: error: `for` loop range of `31` ..  `0` by `-1` is not supported.
* wiz: failed with 2 error(s).

Notes

Original file to reproduce the issue
bugtest.txt
Build from b25ba2d90288fc8a380d99b3ad08a5b48caaa484 commit
Build with wiz\wiz.exe src/bugtest.wiz -o bugtest.nes command

[Bananattack]

Just adding a few notes. It's been a while since implementing this, so trying to log a bit of things about behaviour as the bug is still open.

1. a for loop that counts down from 31 .. 1 by -1 works ok.
2. the loop is always done as a do .. while loop, and the range bounds are inclusive, so the value of the counter after the loop is "supposed to be" the value one-past-the-end of the range.
3. First, the increment/decrement is done, then the compare-and-branch, or just branch.
4. it doesn't have code to check the negative flag as a side-effect of the loop counter decrement.
5. it doesn't seem to be able to do a comparison instruction either, probably because at compile-time it uses a signed integers for the loop bound, and it doesn't cast the result to the type of the counter.

To be honest, I kind of want to improve range literals, so that excluding end bounds is possible, and translating code is easier. I also want to fix for loops to be more usable in general, there's already an issue to allow runtime ranges in for loops too. I'll have to see about that, but at the very least:
a) the negative flag should be checked, and
b) failing that, comparison for loop wraparound should be possible for countdown loops.
c) decrement-and-branch instruction support should be possible for systems that have it such as the Z80.

Anyway, this is the code responsible for most of the logic around for loops, it could certainly be improved in terms of how it builds its loops:

wiz/src/wiz/compiler/compiler.cpp

Lines 6598 to 6748 in a174205

	case StatementKind::For: {
	const auto& forStatement = statement->for_;
	if (currentBank == nullptr) {
	report->error(statement->getDescription().toString() + " must be inside an `in` statement", statement->location);
	break;
	}
	const auto oldContinueLabel = continueLabel;
	const auto oldBreakLabel = breakLabel;
	const auto onExit = makeScopeGuard([&]() {
	continueLabel = oldContinueLabel;
	breakLabel = oldBreakLabel;
	});
	const auto reducedCounter = expressionPool.add(reduceExpression(forStatement.counter.get()));
	const auto reducedSequence = expressionPool.add(reduceExpression(forStatement.sequence.get()));
	if (!reducedCounter || !reducedSequence) {
	break;
	}
	if (reducedSequence->kind != ExpressionKind::RangeLiteral || reducedSequence->info->context == EvaluationContext::RunTime) {
	report->error("`for` loop range must be a compile-time range literal.", statement->location);
	break;
	}
	const auto& rangeLiteral = reducedSequence->tryGet<Expression::RangeLiteral>();
	const auto counterResolvedIdentifierType = reducedCounter->info->type->tryGet<TypeExpression::ResolvedIdentifier>();
	const auto counterBuiltinIntegerType = counterResolvedIdentifierType ? counterResolvedIdentifierType->definition->tryGet<Definition::BuiltinIntegerType>() : nullptr;
	if (!counterBuiltinIntegerType) {
	report->error("`for` loop counter start must be a sized integer type.", statement->location);
	break;
	}
	const auto rangeStart = rangeLiteral->start->tryGet<Expression::IntegerLiteral>();
	const auto rangeEnd = rangeLiteral->end->tryGet<Expression::IntegerLiteral>();
	const auto rangeStep = rangeLiteral->step->tryGet<Expression::IntegerLiteral>();
	if (!rangeStart) {
	report->error("`for` loop range start must be a compile-time integer literal.", statement->location);
	break;
	}
	if (!rangeEnd) {
	report->error("`for` loop range end must be a compile-time integer literal.", statement->location);
	break;
	}
	if (!rangeStep) {
	report->error("`for` loop range step must be a compile-time integer literal.", statement->location);
	break;
	}
	if (rangeStep->value.isZero()) {
	report->error("`for` loop range step must be non-zero.", statement->location);
	break;
	}
	const auto beginLabelDefinition = createAnonymousLabelDefinition("$loop"_sv);
	const auto beginLabelReferenceExpression = expressionPool.add(resolveDefinitionExpression(beginLabelDefinition, {}, statement->location));
	const auto continueLabelDefinition = createAnonymousLabelDefinition("$continue"_sv);
	const auto endLabelDefinition = createAnonymousLabelDefinition("$endloop"_sv);
	continueLabel = continueLabelDefinition;
	breakLabel = endLabelDefinition;
	auto initAssignment = makeFwdUnique<Expression>(
	Expression::BinaryOperator(BinaryOperatorKind::Assignment, reducedCounter->clone(), rangeLiteral->start->clone()),
	reducedCounter->location,
	Optional<ExpressionInfo>());
	auto reducedInitAssignment = expressionPool.add(reduceExpression(initAssignment.get()));
	bool conditionNegated = false;
	const Expression* reducedCondition = nullptr;
	const Instruction* incrementInstruction = nullptr;
	std::vector<InstructionOperandRoot> incrementOperandRoots;
	if (rangeStep->value == Int128(1) || rangeStep->value == Int128(-1)) {
	if (auto destOperand = createOperandFromExpression(reducedCounter, true)) {
	const auto op = rangeStep->value.isPositive() ? UnaryOperatorKind::PreIncrement : UnaryOperatorKind::PreDecrement;
	incrementOperandRoots.reserve(1);
	incrementOperandRoots.push_back(InstructionOperandRoot(reducedCounter, std::move(destOperand)));
	incrementInstruction = builtins.selectInstruction(InstructionType(op), modeFlags, incrementOperandRoots);
	}
	if (incrementInstruction) {
	if ((rangeStep->value.isPositive() && rangeStart->value > rangeEnd->value)
	|| (rangeStep->value.isNegative() && rangeStart->value < rangeEnd->value)) {
	break;
	}
	if ((rangeStep->value.isPositive() && rangeEnd->value == counterBuiltinIntegerType->max)
	|| (rangeStep->value.isNegative() && rangeEnd->value == Int128(1))) {
	if (const auto zeroFlag = platform->getZeroFlag()) {
	const auto& affectedFlags = incrementInstruction->options.affectedFlags;
	if (std::find(affectedFlags.begin(), affectedFlags.end(), zeroFlag) != affectedFlags.end()) {
	conditionNegated = true;
	reducedCondition = expressionPool.add(resolveDefinitionExpression(zeroFlag, {}, statement->location));
	}
	}
	if (!reducedCondition) {
	auto comparisonValue = makeFwdUnique<Expression>(
	Expression::IntegerLiteral(Int128(0)),
	reducedSequence->location,
	ExpressionInfo(EvaluationContext::CompileTime,
	makeFwdUnique<const TypeExpression>(TypeExpression::ResolvedIdentifier(builtins.getDefinition(Builtins::DefinitionType::IExpr)), reducedSequence->location),
	Qualifiers::None));
	auto condition = makeFwdUnique<Expression>(Expression::BinaryOperator(BinaryOperatorKind::NotEqual, reducedCounter->clone(), std::move(comparisonValue)), reducedCounter->location, Optional<ExpressionInfo>());
	reducedCondition = expressionPool.add(reduceExpression(condition.get()));
	}
	} else if (rangeEnd->value >= Int128(0) && rangeEnd->value <= counterBuiltinIntegerType->max) {
	auto comparisonValue = makeFwdUnique<Expression>(
	Expression::IntegerLiteral(rangeEnd->value + rangeStep->value),
	reducedSequence->location,
	ExpressionInfo(EvaluationContext::CompileTime,
	makeFwdUnique<const TypeExpression>(TypeExpression::ResolvedIdentifier(builtins.getDefinition(Builtins::DefinitionType::IExpr)), reducedSequence->location),
	Qualifiers::None));
	auto condition = makeFwdUnique<Expression>(Expression::BinaryOperator(BinaryOperatorKind::NotEqual, reducedCounter->clone(), std::move(comparisonValue)), reducedCounter->location, Optional<ExpressionInfo>());
	reducedCondition = expressionPool.add(reduceExpression(condition.get()));
	}
	}
	}
	if (!reducedCondition) {
	report->error("`for` loop range of `" + rangeStart->value.toString()
	+ "` .. `" + rangeEnd->value.toString() + "`"
	+ (rangeStep->value != Int128(1)
	? " by `" + rangeStep->value.toString() + "`"
	: "")
	+ " is not supported.", reducedSequence->location);
	break;
	}
	if (!incrementInstruction) {
	report->error("could not generate increment instruction for " + statement->getDescription().toString(), statement->location);
	break;
	}
	// TODO: decrement-and-branch optimization if the instruction exists.
	if (!emitExpressionStatementIr(reducedInitAssignment, reducedInitAssignment->location)) {
	report->error("could not generate initial assignment instruction for " + statement->getDescription().toString(), statement->location);
	break;
	}
	irNodes.addNew(IrNode::Label(beginLabelDefinition), statement->location);
	emitStatementIr(forStatement.body.get());
	irNodes.addNew(IrNode::Label(continueLabelDefinition), reducedCondition->location);
	irNodes.addNew(IrNode::Code(incrementInstruction, std::move(incrementOperandRoots)), reducedCondition->location);
	if (!emitBranchIr(forStatement.distanceHint, BranchKind::Goto, beginLabelReferenceExpression, nullptr, conditionNegated, reducedCondition, reducedCondition->location)) {
	report->error("could not generate branch instruction for " + statement->getDescription().toString(), statement->location);
	break;
	}
	irNodes.addNew(IrNode::Label(endLabelDefinition), reducedCondition->location);
	break;
	}

[shinymerlyn]

A few other examples (beyond the initially reported bug) that would be useful for runtime for-loops.
Already covered in @Bananattack's notes above I think, but these are some specific examples that are in the code I'm working on now. Technically they're a separate bug, but while on this broader subject -

Allow the initial value to be set by transfer from another register:

var clear_value: u8 in a = 0;
SomeVariableInABank = clear_value;

y = clear_value;
do {
    SomeArrayVariableInABank[y] = clear_value;
    ++y;
} while !zero;

rewritten:

var clear_value: u8 in a = 0;
SomeVariableInABank = clear_value;

for y in clear_value .. 255 {  // would emit a `tay` instead of `ldy #0`
    SomeArrayVariableInABank[y] = clear_value;
}

---

Allow the initial value for the loop counter register to be set some arbitrary number of lines earlier, with code in between:

y = 7;
// some other code here

do {
    SomeArrayVariableInABank[x] >>>#= 1;
    ++x;
    --y;
} while !zero;

rewritten:

y = 7;
// some other code here

for y in y .. 1 by -1 {  // would not emit a loop init instruction here
    SomeArrayVariableInABank[x] >>>#= 1;
    ++x;
}

[veremenko-y]

for y in clear_value .. 255 { // would emit a tay instead of ldy #0

Looks more like an optimizer bug, not the loop related. I'd recommend creating an issue for this.

for y in y .. 1 by -1 { // would not emit a loop init instruction here

This looks like correct behaviour, unless I'm misunderstanding. You ask compiler not initalize y and take it as is.

[shinymerlyn]

for y in clear_value .. 255 { // would emit a tay instead of ldy #0

Looks more like an optimizer bug, not the loop related. I'd recommend creating an issue for this.

I apologize. The comments aren't explained well enough. This isn't an optimizer problem. It is a missing feature in the compiler, that you can't use a variable as one of the loop bounds.

I want to write this, but the compiler won't let me:

a = 0;
// some code here that uses a
for y in a .. 255 { ... }

The compiler lets me write the following code, and I get for-loop syntax, and I get the right range, but emits more code than I'd prefer for this exact scenario:

a = 0;
// some code here that uses a
for y in 0 .. 255 { ... }

The problem related to an optimization, but it is a hand-optimization problem, not a compiler automated optimization problem.

---

for y in y .. 1 by -1 { // would not emit a loop init instruction here

This looks like correct behaviour, unless I'm misunderstanding. You ask compiler not initalize y and take it as is.

Yeah, again a case of my comments being unclear.

The compiler doesn't let me write this code either. I get this error.

test.wiz:775: error: range start must be a compile-time integer literal

I am replying to this thread because on the discord @Bananattack asked for some more explicit examples, related to their notes on all the types of runtime for loops people would like supported. So I'm adding some in my project that I currently have to work around with do/while loops.

[undisbeliever]

I've looked through my code and I could not find any for loops, all of my loops were do { } while... or while true { }.

Here are three examples of do ... while loops that I would like to see written using for statements (beyond the standard 0 .. 31 inclusive and 8 .. 1 by -1 inclusive loops).

---

A common looping pattern in my code is a backwards loop from the last-index to 0 inclusive. (NOTE: this pattern only works if the initial loop variable is <= 0x80 for 8 bit indexes and <= 0x8000 for 16 bit indexes).

    x = N_ENTITIES_IN_ROOM_DATA - 1;
    do {
        push8(x);

        entities.spawn_entity(a = room.entity_xPos[x],
                              a = room.entity_yPos[x],
                              a = room.entity_type[x],
                              y = room.entity_parameter[x]);

        x = pop8();

        x--;
    } while !negative;

I also have a lot of loops that increment or decrement by 2 to access word arrays and structure-of-word-arrays data.

    // Loop through the entity structure-of-word-arrays
    // Loop through LAST_INDEX to 0 inclusive, decrementing by 2
    yy = (N_ENTITIES - 1) * 2;
    ^do {
        a = entities.SoA.shadowSize[unaligned yy] as u8;

        // ...

        yy--;
        yy--;
    } while !negative;

Countdown loops using a zeropage variable.

    _columnsLeft = a = MAP_WIDTH;
    do {
        y = map[x];

        *(far &snes.ppu.vram_write_data_l) = a = tileset.bottomLeft_low[y];
        *(far &snes.ppu.vram_write_data_h) = a = tileset.bottomLeft_high[y];
        *(far &snes.ppu.vram_write_data_l) = a = tileset.bottomRight_low[y];
        *(far &snes.ppu.vram_write_data_h) = a = tileset.bottomRight_high[y];
        x++;

        _columnsLeft--;
    } while !zero;

[veremenko-y]

Yeah, again a case of my comments being unclear.
I am replying to this thread because on the discord Bananattack asked for some more explicit examples

Got it, sorry about that, I did indeed misunderstood. Also I haven't been following up on the Discord lately.

Not sure what Bananattack wants, but if I were him, I'd like those to be separate feature requests on Github, it'd be easier to track down, and they could close it if needed too.