Skip to content

Commit 7608a9c

Browse files
ASDAlexander77claude
ASDAlexander77
and
claude
committed
Make conditionalExpressionLowering nesting-safe; fix stale 01tuple.ts assertion
conditionalExpressionLowering (CodeLogicHelper.h) branched into its result block from the then/else block handles captured before invoking the builder callbacks, assuming the insertion point never moved. That broke as soon as a callback itself called the helper again (needed for AnyCompareOp's new multi-way coercion dispatch), producing an "operation with block successors must terminate its parent block" verifier error. It now re-reads the actual insertion block after each builder runs, so it composes safely when nested. AnyCompareOpLowering's local workaround (nestableConditional) is removed in favor of the shared, now-fixed helper. 01tuple.ts's `assert(obj8.field1 === 10)` relied on the old, buggy === semantics that coerced like == (fixed in the previous commit) -- field1 is string-typed and holds the coerced "10", so strict equality against the number 10 is correctly false. Updated to match the coercion pattern already used elsewhere in the same file. Full suite: 700/700 passing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent 1b196cc commit 7608a9c

3 files changed

Lines changed: 49 additions & 86 deletions

File tree

‎tslang/include/TypeScript/LowerToLLVM/CodeLogicHelper.h‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -114,19 +114,25 @@ class CodeLogicHelper
114114
// then block
115115
auto *thenBlock = rewriter.createBlock(continuationBlock);
116116
auto thenValue = thenBuilder(rewriter, loc);
117+
// thenBuilder may itself branch into further blocks (e.g. a nested
118+
// conditionalExpressionLowering call) -- always branch to the result block
119+
// from wherever the insertion point actually ended up, not from the
120+
// (possibly stale) block handle captured before the builder ran.
121+
auto *thenEndBlock = rewriter.getInsertionBlock();
117122

118123
// else block
119124
auto *elseBlock = rewriter.createBlock(continuationBlock);
120125
auto elseValue = elseBuilder(rewriter, loc);
126+
auto *elseEndBlock = rewriter.getInsertionBlock();
121127

122128
// result block
123129
auto *resultBlock = rewriter.createBlock(continuationBlock, TypeRange{type}, {loc});
124130
rewriter.create<LLVM::BrOp>(loc, ValueRange{}, continuationBlock);
125131

126-
rewriter.setInsertionPointToEnd(thenBlock);
132+
rewriter.setInsertionPointToEnd(thenEndBlock);
127133
rewriter.create<LLVM::BrOp>(loc, ValueRange{thenValue}, resultBlock);
128134

129-
rewriter.setInsertionPointToEnd(elseBlock);
135+
rewriter.setInsertionPointToEnd(elseEndBlock);
130136
rewriter.create<LLVM::BrOp>(loc, ValueRange{elseValue}, resultBlock);
131137

132138
// Generate assertion test.

‎tslang/lib/TypeScript/LowerToLLVM.cpp‎

Lines changed: 40 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -742,49 +742,6 @@ class AnyCompareOpLowering : public TsLlvmPattern<mlir_ts::AnyCompareOp>
742742
public:
743743
using TsLlvmPattern<mlir_ts::AnyCompareOp>::TsLlvmPattern;
744744

745-
// CodeLogicHelper::conditionalExpressionLowering assumes its then/else builders
746-
// leave the insertion point untouched (it branches from the *original* then/else
747-
// block handles into the result block). That assumption breaks the moment a
748-
// builder itself calls the helper again -- the nested call moves the insertion
749-
// point to its own continuation block, so the outer helper's branch lands in the
750-
// wrong (already-terminated) block. This variant re-reads the insertion block
751-
// after each builder runs, so it composes safely when nested.
752-
mlir::Value nestableConditional(mlir::Location loc, mlir::Type type, ConversionPatternRewriter &rewriter,
753-
mlir::Value condition, mlir::function_ref<mlir::Value(ConversionPatternRewriter &, Location)> thenBuilder,
754-
mlir::function_ref<mlir::Value(ConversionPatternRewriter &, Location)> elseBuilder) const
755-
{
756-
auto *opBlock = rewriter.getInsertionBlock();
757-
auto opPosition = rewriter.getInsertionPoint();
758-
auto *continuationBlock = rewriter.splitBlock(opBlock, opPosition);
759-
760-
auto *thenBlock = rewriter.createBlock(continuationBlock);
761-
rewriter.setInsertionPointToStart(thenBlock);
762-
auto thenValue = thenBuilder(rewriter, loc);
763-
auto *thenEndBlock = rewriter.getInsertionBlock();
764-
765-
auto *elseBlock = rewriter.createBlock(continuationBlock);
766-
rewriter.setInsertionPointToStart(elseBlock);
767-
auto elseValue = elseBuilder(rewriter, loc);
768-
auto *elseEndBlock = rewriter.getInsertionBlock();
769-
770-
auto *resultBlock = rewriter.createBlock(continuationBlock, TypeRange{type}, {loc});
771-
rewriter.setInsertionPointToEnd(resultBlock);
772-
rewriter.create<LLVM::BrOp>(loc, ValueRange{}, continuationBlock);
773-
774-
rewriter.setInsertionPointToEnd(thenEndBlock);
775-
rewriter.create<LLVM::BrOp>(loc, ValueRange{thenValue}, resultBlock);
776-
777-
rewriter.setInsertionPointToEnd(elseEndBlock);
778-
rewriter.create<LLVM::BrOp>(loc, ValueRange{elseValue}, resultBlock);
779-
780-
rewriter.setInsertionPointToEnd(opBlock);
781-
rewriter.create<LLVM::CondBrOp>(loc, condition, thenBlock, elseBlock);
782-
783-
rewriter.setInsertionPointToStart(continuationBlock);
784-
785-
return resultBlock->getArguments().front();
786-
}
787-
788745
// same-representation compare: valid when both operands are known to hold the
789746
// same underlying kind (used both when the loose == / != tags actually match, and
790747
// for ===/!==/relational ops, which never coerce across kinds).
@@ -806,9 +763,9 @@ class AnyCompareOpLowering : public TsLlvmPattern<mlir_ts::AnyCompareOp>
806763
auto dataPtr1 = al.getDataPtrOfAny(op1);
807764
auto dataPtr2 = al.getDataPtrOfAny(op2);
808765

809-
return nestableConditional(
810-
loc, th.getLLVMBoolType(), rewriter, sizesEqual,
811-
[&](ConversionPatternRewriter &rewriter, Location loc) {
766+
return clh.conditionalExpressionLowering(
767+
loc, th.getLLVMBoolType(), sizesEqual,
768+
[&](OpBuilder &builder, Location loc) {
812769
auto const0 = clh.createI32ConstantOf(0);
813770
// sizeAny1 equals sizeAny2
814771
auto compareResult =
@@ -851,7 +808,7 @@ class AnyCompareOpLowering : public TsLlvmPattern<mlir_ts::AnyCompareOp>
851808

852809
return bodyCmpResult;
853810
},
854-
[&](ConversionPatternRewriter &rewriter, Location loc) {
811+
[&](OpBuilder &builder, Location loc) {
855812
// sizes (and therefore kinds) differ: == is false, != is true; ordering
856813
// ops and === / !== fall back to their pre-existing "false" behavior.
857814
if (code == SyntaxKind::ExclamationEqualsToken)
@@ -909,14 +866,14 @@ class AnyCompareOpLowering : public TsLlvmPattern<mlir_ts::AnyCompareOp>
909866
return storedTy == numberTy ? raw : castLogic.cast(raw, storedTy, numberTy);
910867
};
911868

912-
return nestableConditional(
913-
loc, th.getF64Type(), rewriter, isTag(numberTag, "number"),
914-
[&](ConversionPatternRewriter &, Location) { return asF64(numberTy); },
915-
[&](ConversionPatternRewriter &rewriter, Location) {
916-
return nestableConditional(
917-
loc, th.getF64Type(), rewriter, isTag(numberTag, "s64"),
918-
[&](ConversionPatternRewriter &, Location) { return asF64(mlir::IntegerType::get(rewriter.getContext(), 64, mlir::IntegerType::Signed)); },
919-
[&](ConversionPatternRewriter &, Location) { return asF64(mlir::IntegerType::get(rewriter.getContext(), 32, mlir::IntegerType::Signed)); });
869+
return clh.conditionalExpressionLowering(
870+
loc, th.getF64Type(), isTag(numberTag, "number"),
871+
[&](OpBuilder &, Location) { return asF64(numberTy); },
872+
[&](OpBuilder &, Location) {
873+
return clh.conditionalExpressionLowering(
874+
loc, th.getF64Type(), isTag(numberTag, "s64"),
875+
[&](OpBuilder &, Location) { return asF64(mlir::IntegerType::get(rewriter.getContext(), 64, mlir::IntegerType::Signed)); },
876+
[&](OpBuilder &, Location) { return asF64(mlir::IntegerType::get(rewriter.getContext(), 32, mlir::IntegerType::Signed)); });
920877
});
921878
};
922879

@@ -971,30 +928,30 @@ class AnyCompareOpLowering : public TsLlvmPattern<mlir_ts::AnyCompareOp>
971928
auto op1IsBooleanOp2IsString = rewriter.create<LLVM::AndOp>(loc, tag1IsBoolean, tag2IsString);
972929
auto op1IsStringOp2IsBoolean = rewriter.create<LLVM::AndOp>(loc, tag1IsString, tag2IsBoolean);
973930

974-
return nestableConditional(
975-
loc, th.getLLVMBoolType(), rewriter, op1IsNumberOp2IsString,
976-
[&](ConversionPatternRewriter &, Location loc) { return numberStringCase(op1, tag1, op2); },
977-
[&](ConversionPatternRewriter &rewriter, Location loc) {
978-
return nestableConditional(
979-
loc, th.getLLVMBoolType(), rewriter, op1IsStringOp2IsNumber,
980-
[&](ConversionPatternRewriter &, Location loc) { return numberStringCase(op2, tag2, op1); },
981-
[&](ConversionPatternRewriter &rewriter, Location loc) {
982-
return nestableConditional(
983-
loc, th.getLLVMBoolType(), rewriter, op1IsBooleanOp2IsNumber,
984-
[&](ConversionPatternRewriter &, Location loc) { return booleanNumberCase(op1, op2, tag2); },
985-
[&](ConversionPatternRewriter &rewriter, Location loc) {
986-
return nestableConditional(
987-
loc, th.getLLVMBoolType(), rewriter, op1IsNumberOp2IsBoolean,
988-
[&](ConversionPatternRewriter &, Location loc) { return booleanNumberCase(op2, op1, tag1); },
989-
[&](ConversionPatternRewriter &rewriter, Location loc) {
990-
return nestableConditional(
991-
loc, th.getLLVMBoolType(), rewriter, op1IsBooleanOp2IsString,
992-
[&](ConversionPatternRewriter &, Location loc) { return booleanStringCase(op1, op2); },
993-
[&](ConversionPatternRewriter &rewriter, Location loc) {
994-
return nestableConditional(
995-
loc, th.getLLVMBoolType(), rewriter, op1IsStringOp2IsBoolean,
996-
[&](ConversionPatternRewriter &, Location loc) { return booleanStringCase(op2, op1); },
997-
[&](ConversionPatternRewriter &rewriter, Location loc) {
931+
return clh.conditionalExpressionLowering(
932+
loc, th.getLLVMBoolType(), op1IsNumberOp2IsString,
933+
[&](OpBuilder &, Location loc) { return numberStringCase(op1, tag1, op2); },
934+
[&](OpBuilder &builder, Location loc) {
935+
return clh.conditionalExpressionLowering(
936+
loc, th.getLLVMBoolType(), op1IsStringOp2IsNumber,
937+
[&](OpBuilder &, Location loc) { return numberStringCase(op2, tag2, op1); },
938+
[&](OpBuilder &builder, Location loc) {
939+
return clh.conditionalExpressionLowering(
940+
loc, th.getLLVMBoolType(), op1IsBooleanOp2IsNumber,
941+
[&](OpBuilder &, Location loc) { return booleanNumberCase(op1, op2, tag2); },
942+
[&](OpBuilder &builder, Location loc) {
943+
return clh.conditionalExpressionLowering(
944+
loc, th.getLLVMBoolType(), op1IsNumberOp2IsBoolean,
945+
[&](OpBuilder &, Location loc) { return booleanNumberCase(op2, op1, tag1); },
946+
[&](OpBuilder &builder, Location loc) {
947+
return clh.conditionalExpressionLowering(
948+
loc, th.getLLVMBoolType(), op1IsBooleanOp2IsString,
949+
[&](OpBuilder &, Location loc) { return booleanStringCase(op1, op2); },
950+
[&](OpBuilder &builder, Location loc) {
951+
return clh.conditionalExpressionLowering(
952+
loc, th.getLLVMBoolType(), op1IsStringOp2IsBoolean,
953+
[&](OpBuilder &, Location loc) { return booleanStringCase(op2, op1); },
954+
[&](OpBuilder &builder, Location loc) {
998955
// no known coercion: kinds differ and are unequal
999956
return (mlir::Value)clh.createI1ConstantOf(!isEquals);
1000957
});
@@ -1040,10 +997,10 @@ class AnyCompareOpLowering : public TsLlvmPattern<mlir_ts::AnyCompareOp>
1040997
auto const0 = clh.createI32ConstantOf(0);
1041998
auto tagsEqual = rewriter.create<LLVM::ICmpOp>(loc, LLVM::ICmpPredicate::eq, tagCmp.getResult(), const0);
1042999

1043-
auto result = nestableConditional(
1044-
loc, th.getLLVMBoolType(), rewriter, tagsEqual,
1045-
[&](ConversionPatternRewriter &rewriter, Location loc) { return sameKindCompare(op, op1, op2, code, loc, al, clh, th, ch, llvmtch, rewriter); },
1046-
[&](ConversionPatternRewriter &rewriter, Location loc) { return coerceAndCompareMixedKinds(op1, op2, tag1, tag2, code, loc, al, castLogic, clh, th, ch, tch, rewriter); });
1000+
auto result = clh.conditionalExpressionLowering(
1001+
loc, th.getLLVMBoolType(), tagsEqual,
1002+
[&](OpBuilder &builder, Location loc) { return sameKindCompare(op, op1, op2, code, loc, al, clh, th, ch, llvmtch, rewriter); },
1003+
[&](OpBuilder &builder, Location loc) { return coerceAndCompareMixedKinds(op1, op2, tag1, tag2, code, loc, al, castLogic, clh, th, ch, tch, rewriter); });
10471004

10481005
rewriter.replaceOp(op, result);
10491006

‎tslang/test/tester/tests/01tuple.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ function main() {
3737
const obj8 : IObj = { field1: 10 };
3838
print(obj8.field1);
3939

40-
assert(obj8.field1 === 10);
40+
assert(obj8.field1 === "10");
4141

4242
print("done.");
4343
}

0 commit comments

Comments
 (0)