Skip to content

Commit

Permalink
Some of the convert optimizations were incorrect. For example, the se… (
Browse files Browse the repository at this point in the history
#1419)

* Some of the convert optimizations were incorrect. For example, the sequence
```mlir
  %1 = fir.convert %0 : (i32) -> i8
  %2 = fir.convert %1 : (i8) -> i16
```
must be preserved since the first truncation can modify the observed
value that is extended in the second op.

Refactor the transformations to catch this case, extend cases that were
not being caught, and deal with errors with respect to index type.

Fix tests that now had bugs because of missing truncations.

* Add peephole tests

* Use latest and greatest clang-format.
  • Loading branch information
schweitzpgi authored Jan 27, 2022
1 parent de70e16 commit fb8eb63
Show file tree
Hide file tree
Showing 5 changed files with 587 additions and 103 deletions.
77 changes: 64 additions & 13 deletions flang/include/flang/Optimizer/Dialect/CanonicalizationPatterns.td
Original file line number Diff line number Diff line change
Expand Up @@ -23,28 +23,80 @@ def IdenticalTypePred : Constraint<CPred<"$0.getType() == $1.getType()">>;
def IntegerTypePred : Constraint<CPred<"fir::isa_integer($0.getType())">>;
def IndexTypePred : Constraint<CPred<"$0.getType().isa<mlir::IndexType>()">>;

def SmallerWidthPred
: Constraint<CPred<"$0.getType().getIntOrFloatBitWidth() "
"<= $1.getType().getIntOrFloatBitWidth()">>;
// Widths are monotonic.
// $0.bits >= $1.bits >= $2.bits or $0.bits <= $1.bits <= $2.bits
def MonotonicTypePred
: Constraint<CPred<"(($0.getType().isa<mlir::IntegerType>() && "
" $1.getType().isa<mlir::IntegerType>() && "
" $2.getType().isa<mlir::IntegerType>()) || "
" ($0.getType().isa<mlir::FloatType>() && "
" $1.getType().isa<mlir::FloatType>() && "
" $2.getType().isa<mlir::FloatType>())) && "
"(($0.getType().getIntOrFloatBitWidth() <= "
" $1.getType().getIntOrFloatBitWidth() && "
" $1.getType().getIntOrFloatBitWidth() <= "
" $2.getType().getIntOrFloatBitWidth()) || "
" ($0.getType().getIntOrFloatBitWidth() >= "
" $1.getType().getIntOrFloatBitWidth() && "
" $1.getType().getIntOrFloatBitWidth() >= "
" $2.getType().getIntOrFloatBitWidth()))">>;

def IntPred : Constraint<CPred<
"$0.getType().isa<mlir::IntegerType>() && "
"$1.getType().isa<mlir::IntegerType>()">>;

// If both are int type and the first is smaller than the second.
// $0.bits <= $1.bits
def SmallerWidthPred : Constraint<CPred<
"$0.getType().getIntOrFloatBitWidth() <= "
"$1.getType().getIntOrFloatBitWidth()">>;
def StrictSmallerWidthPred : Constraint<CPred<
"$0.getType().getIntOrFloatBitWidth() < "
"$1.getType().getIntOrFloatBitWidth()">>;

// floats or ints that undergo successive extensions or successive truncations.
def ConvertConvertOptPattern
: Pat<(fir_ConvertOp (fir_ConvertOp $arg)),
: Pat<(fir_ConvertOp:$res (fir_ConvertOp:$irm $arg)),
(fir_ConvertOp $arg),
[(MonotonicTypePred $res, $irm, $arg)]>;

// Widths are increasingly monotonic to type index, so there is no
// possibility of a truncation before the conversion to index.
// $res == index && $irm.bits >= $arg.bits
def ConvertAscendingIndexOptPattern
: Pat<(fir_ConvertOp:$res (fir_ConvertOp:$irm $arg)),
(fir_ConvertOp $arg),
[(IndexTypePred $res), (IntPred $irm, $arg),
(SmallerWidthPred $arg, $irm)]>;

// Widths are decreasingly monotonic from type index, so the truncations
// continue to lop off more bits.
// $arg == index && $res.bits < $irm.bits
def ConvertDescendingIndexOptPattern
: Pat<(fir_ConvertOp:$res (fir_ConvertOp:$irm $arg)),
(fir_ConvertOp $arg),
[(IntegerTypePred $arg)]>;
[(IndexTypePred $arg), (IntPred $irm, $res),
(SmallerWidthPred $res, $irm)]>;

// Useless convert to exact same type.
def RedundantConvertOptPattern
: Pat<(fir_ConvertOp:$res $arg),
(replaceWithValue $arg),
[(IdenticalTypePred $res, $arg)
,(IntegerTypePred $arg)]>;
[(IdenticalTypePred $res, $arg)]>;

// Useless extension followed by truncation to get same width integer.
def CombineConvertOptPattern
: Pat<(fir_ConvertOp:$res(fir_ConvertOp:$irm $arg)),
(replaceWithValue $arg),
[(IdenticalTypePred $res, $arg)
,(IntegerTypePred $arg)
,(IntegerTypePred $irm)
,(SmallerWidthPred $arg, $irm)]>;
[(IntPred $res, $arg), (IdenticalTypePred $res, $arg),
(IntPred $arg, $irm), (SmallerWidthPred $arg, $irm)]>;

// Useless extension followed by truncation to get smaller width integer.
def CombineConvertTruncOptPattern
: Pat<(fir_ConvertOp:$res(fir_ConvertOp:$irm $arg)),
(fir_ConvertOp $arg),
[(IntPred $res, $arg), (StrictSmallerWidthPred $res, $arg),
(IntPred $arg, $irm), (SmallerWidthPred $arg, $irm)]>;

def createConstantOp
: NativeCodeCall<"$_builder.create<mlir::arith::ConstantOp>"
Expand All @@ -55,7 +107,6 @@ def createConstantOp
def ForwardConstantConvertPattern
: Pat<(fir_ConvertOp:$res (Arith_ConstantOp:$cnt $attr)),
(createConstantOp $res, $attr),
[(IndexTypePred $res)
,(IntegerTypePred $cnt)]>;
[(IndexTypePred $res), (IntegerTypePred $cnt)]>;

#endif // FORTRAN_FIR_REWRITE_PATTERNS
7 changes: 4 additions & 3 deletions flang/lib/Optimizer/Dialect/FIROps.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -806,9 +806,10 @@ static mlir::LogicalResult verify(fir::ConstcOp &op) {

void fir::ConvertOp::getCanonicalizationPatterns(
OwningRewritePatternList &results, MLIRContext *context) {
results.insert<ConvertConvertOptPattern, RedundantConvertOptPattern,
CombineConvertOptPattern, ForwardConstantConvertPattern>(
context);
results.insert<ConvertConvertOptPattern, ConvertAscendingIndexOptPattern,
ConvertDescendingIndexOptPattern, RedundantConvertOptPattern,
CombineConvertOptPattern, CombineConvertTruncOptPattern,
ForwardConstantConvertPattern>(context);
}

mlir::OpFoldResult fir::ConvertOp::fold(llvm::ArrayRef<mlir::Attribute> opnds) {
Expand Down
126 changes: 126 additions & 0 deletions flang/test/Fir/peephole.fir
Original file line number Diff line number Diff line change
@@ -0,0 +1,126 @@
// RUN: tco %s | FileCheck %s

// Test peephole optimizations

// CHECK-LABEL: define i8 @test_trunc(
// CHECK-SAME: i256 %[[arg:.*]])
// CHECK-NEXT: = trunc i256 %[[arg]] to i8
// CHECK-NEXT: ret i8
func @test_trunc(%0 : i256) -> i8 {
%1 = fir.convert %0 : (i256) -> i128
%2 = fir.convert %1 : (i128) -> i64
%3 = fir.convert %2 : (i64) -> i32
%4 = fir.convert %3 : (i32) -> i16
%5 = fir.convert %4 : (i16) -> i8
return %5 : i8
}

// CHECK-LABEL: define i256 @test_sext(
// CHECK-SAME: i8 %[[arg:.*]])
// CHECK-NEXT: = sext i8 %[[arg]] to i256
// CHECK-NEXT: ret i256
func @test_sext(%0 : i8) -> i256 {
%1 = fir.convert %0 : (i8) -> i16
%2 = fir.convert %1 : (i16) -> i32
%3 = fir.convert %2 : (i32) -> i64
%4 = fir.convert %3 : (i64) -> i128
%5 = fir.convert %4 : (i128) -> i256
return %5 : i256
}

// CHECK-LABEL: define half @test_fptrunc(
// CHECK-SAME: fp128 %[[arg:.*]])
// CHECK-NEXT: %[[res:.*]] = fptrunc fp128 %[[arg]] to half
// CHECK-NEXT: ret half %[[res]]
func @test_fptrunc(%0 : f128) -> f16 {
%2 = fir.convert %0 : (f128) -> f64
%3 = fir.convert %2 : (f64) -> f32
%4 = fir.convert %3 : (f32) -> f16
return %4 : f16
}

// CHECK-LABEL: define x86_fp80 @test_fpext(
// CHECK-SAME: bfloat %[[arg:.*]])
// CHECK-NEXT: = fpext bfloat %[[arg]] to x86_fp80
// CHECK-NEXT: ret x86_fp80
func @test_fpext(%0 : bf16) -> f80 {
%2 = fir.convert %0 : (bf16) -> f32
%3 = fir.convert %2 : (f32) -> f64
%4 = fir.convert %3 : (f64) -> f80
return %4 : f80
}

// CHECK-LABEL: define i64 @test_ascending(
// CHECK-SAME: i8 %[[arg:.*]])
// CHECK-NEXT: = sext i8 %[[arg]] to i64
// CHECK-NEXT: ret i64
func @test_ascending(%0 : i8) -> index {
%1 = fir.convert %0 : (i8) -> i16
%2 = fir.convert %1 : (i16) -> i32
%3 = fir.convert %2 : (i32) -> i64
%5 = fir.convert %3 : (i64) -> index
return %5 : index
}

// CHECK-LABEL: define i8 @test_descending(
// CHECK-SAME: i64 %[[arg:.*]])
// CHECK-NEXT: = trunc i64 %[[arg]] to i8
// CHECK-NEXT: ret i8
func @test_descending(%0 : index) -> i8 {
%2 = fir.convert %0 : (index) -> i64
%3 = fir.convert %2 : (i64) -> i32
%4 = fir.convert %3 : (i32) -> i16
%5 = fir.convert %4 : (i16) -> i8
return %5 : i8
}

// CHECK-LABEL: define float @test_useless(
// CHECK-SAME: float %[[arg:.*]])
// CHECK-NEXT: ret float %[[arg]]
func @test_useless(%0 : f32) -> f32 {
%1 = fir.convert %0 : (f32) -> f32
return %1 : f32
}

// CHECK-LABEL: define float @test_useless_sext(
// CHECK-SAME: i32 %[[arg:.*]])
// CHECK-NEXT: %[[res:.*]] = sitofp i32 %[[arg]] to float
// CHECK-NEXT: ret float %[[res]]
func @test_useless_sext(%0 : i32) -> f32 {
%1 = fir.convert %0 : (i32) -> i64
%2 = fir.convert %1 : (i64) -> i32
%3 = fir.convert %2 : (i32) -> f32
return %3 : f32
}

// CHECK-LABEL: define i16 @test_hump(
// CHECK-SAME: i32 %[[arg:.*]])
// CHECK-NEXT: trunc i32 %[[arg]] to i16
// CHECK-NEXT: ret i16
func @test_hump(%0 : i32) -> i16 {
%1 = fir.convert %0 : (i32) -> i64
%2 = fir.convert %1 : (i64) -> i16
return %2 : i16
}

// CHECK-LABEL: define i16 @test_slump(
// CHECK-SAME: i32 %[[arg:.*]])
// CHECK-NEXT: %[[i:.*]] = trunc i32 %[[arg]] to i8
// CHECK-NEXT: sext i8 %[[i]] to i16
// CHECK-NEXT: ret i16
func @test_slump(%0 : i32) -> i16 {
%1 = fir.convert %0 : (i32) -> i8
%2 = fir.convert %1 : (i8) -> i16
return %2 : i16
}

// CHECK-LABEL: define i64 @test_slump2(
// CHECK-SAME: i64 %[[arg:.*]])
// CHECK-NEXT: %[[i:.*]] = trunc i64 %[[arg]] to i16
// CHECK-NEXT: sext i16 %[[i]] to i64
// CHECK-NEXT: ret i64
func @test_slump2(%0 : index) -> index {
%1 = fir.convert %0 : (index) -> i16
%2 = fir.convert %1 : (i16) -> index
return %2 : index
}
91 changes: 49 additions & 42 deletions flang/test/Lower/array-elemental-calls-char.f90
Original file line number Diff line number Diff line change
Expand Up @@ -206,49 +206,56 @@ elemental function elem_return_char(c)
end function

! CHECK-LABEL: func @_QMchar_elemPfoo6(
! CHECK-SAME: %[[VAL_0:.*]]: !fir.boxchar<1>{{.*}}) {
! CHECK-SAME: %[[VAL_0:.*]]: !fir.boxchar<1> {fir.bindc_name = "c"}) {
subroutine foo6(c)
! CHECK-DAG: %[[VAL_1:.*]] = arith.constant false
! CHECK-DAG: %[[VAL_2:.*]] = arith.constant 32 : i8
! CHECK-DAG: %[[VAL_3:.*]] = arith.constant 10 : index
! CHECK-DAG: %[[VAL_4:.*]] = arith.constant 0 : index
! CHECK-DAG: %[[VAL_5:.*]] = arith.constant 1 : index
! CHECK: %[[VAL_6:.*]]:2 = fir.unboxchar %[[VAL_0]] : (!fir.boxchar<1>) -> (!fir.ref<!fir.char<1,?>>, index)
! CHECK: %[[VAL_7:.*]] = fir.convert %[[VAL_6]]#0 : (!fir.ref<!fir.char<1,?>>) -> !fir.ref<!fir.array<10x!fir.char<1,?>>>
! CHECK: %[[VAL_8:.*]] = fir.shape %[[VAL_3]] : (index) -> !fir.shape<1>
! CHECK: br ^bb1(%[[VAL_4]], %[[VAL_3]] : index, index)
! CHECK: ^bb1(%[[VAL_9:.*]]: index, %[[VAL_10:.*]]: index):
! CHECK: %[[VAL_11:.*]] = arith.cmpi sgt, %[[VAL_10]], %[[VAL_4]] : index
! CHECK: cond_br %[[VAL_11]], ^bb2, ^bb6
! CHECK: ^bb2:
! CHECK: %[[VAL_12:.*]] = arith.addi %[[VAL_9]], %[[VAL_5]] : index
! CHECK: %[[VAL_13:.*]] = fir.array_coor %[[VAL_7]](%[[VAL_8]]) %[[VAL_12]] typeparams %[[VAL_6]]#1 : (!fir.ref<!fir.array<10x!fir.char<1,?>>>, !fir.shape<1>, index, index) -> !fir.ref<!fir.char<1,?>>
! CHECK: %[[VAL_14:.*]] = fir.emboxchar %[[VAL_13]], %[[VAL_6]]#1 : (!fir.ref<!fir.char<1,?>>, index) -> !fir.boxchar<1>
! CHECK: %[[VAL_15:.*]] = fir.alloca !fir.char<1,?>(%[[VAL_6]]#1 : index) {bindc_name = ".result"}
! CHECK: %[[VAL_16:.*]] = fir.call @_QMchar_elemPelem_return_char(%[[VAL_15]], %[[VAL_6]]#1, %[[VAL_14]]) : (!fir.ref<!fir.char<1,?>>, index, !fir.boxchar<1>) -> !fir.boxchar<1>
! CHECK: %[[VAL_17:.*]] = fir.convert %[[VAL_6]]#1 : (index) -> i64
! CHECK: %[[VAL_18:.*]] = fir.convert %[[VAL_13]] : (!fir.ref<!fir.char<1,?>>) -> !fir.ref<i8>
! CHECK: %[[VAL_19:.*]] = fir.convert %[[VAL_15]] : (!fir.ref<!fir.char<1,?>>) -> !fir.ref<i8>
! CHECK: fir.call @llvm.memmove.p0i8.p0i8.i64(%[[VAL_18]], %[[VAL_19]], %[[VAL_17]], %[[VAL_1]]) : (!fir.ref<i8>, !fir.ref<i8>, i64, i1) -> ()
! CHECK: %[[VAL_20:.*]] = arith.subi %[[VAL_6]]#1, %[[VAL_5]] : index
! CHECK: %[[VAL_21:.*]] = fir.undefined !fir.char<1>
! CHECK: %[[VAL_22:.*]] = fir.insert_value %[[VAL_21]], %[[VAL_2]], [0 : index] : (!fir.char<1>, i8) -> !fir.char<1>
! CHECK: %[[VAL_23:.*]] = arith.subi %[[VAL_20]], %[[VAL_6]]#1 : index
! CHECK: %[[VAL_24:.*]] = arith.addi %[[VAL_23]], %[[VAL_5]] : index
! CHECK: br ^bb3(%[[VAL_6]]#1, %[[VAL_24]] : index, index)
! CHECK: ^bb3(%[[VAL_25:.*]]: index, %[[VAL_26:.*]]: index):
! CHECK: %[[VAL_27:.*]] = arith.cmpi sgt, %[[VAL_26]], %[[VAL_4]] : index
! CHECK: cond_br %[[VAL_27]], ^bb4, ^bb5
! CHECK: ^bb4:
! CHECK: %[[VAL_28:.*]] = fir.convert %[[VAL_13]] : (!fir.ref<!fir.char<1,?>>) -> !fir.ref<!fir.array<?x!fir.char<1>>>
! CHECK: %[[VAL_29:.*]] = fir.coordinate_of %[[VAL_28]], %[[VAL_25]] : (!fir.ref<!fir.array<?x!fir.char<1>>>, index) -> !fir.ref<!fir.char<1>>
! CHECK: fir.store %[[VAL_22]] to %[[VAL_29]] : !fir.ref<!fir.char<1>>
! CHECK: %[[VAL_30:.*]] = arith.addi %[[VAL_25]], %[[VAL_5]] : index
! CHECK: %[[VAL_31:.*]] = arith.subi %[[VAL_26]], %[[VAL_5]] : index
! CHECK: br ^bb3(%[[VAL_30]], %[[VAL_31]] : index, index)
! CHECK: ^bb5:
! CHECK: %[[VAL_32:.*]] = arith.subi %[[VAL_10]], %[[VAL_5]] : index
! CHECK: br ^bb1(%[[VAL_12]], %[[VAL_32]] : index, index)
! CHECK: %[[VAL_1:.*]] = arith.constant 10 : index
! CHECK: %[[VAL_2:.*]] = arith.constant 1 : index
! CHECK: %[[VAL_3:.*]] = arith.constant 0 : index
! CHECK: %[[VAL_4:.*]] = arith.constant false
! CHECK: %[[VAL_5:.*]] = arith.constant 32 : i8
! CHECK: %[[VAL_6:.*]]:2 = fir.unboxchar %[[VAL_0]] : (!fir.boxchar<1>) -> (!fir.ref<!fir.char<1,?>>, index)
! CHECK: %[[VAL_7:.*]] = fir.convert %[[VAL_6]]#0 : (!fir.ref<!fir.char<1,?>>) -> !fir.ref<!fir.array<10x!fir.char<1,?>>>
! CHECK: %[[VAL_8:.*]] = fir.shape %[[VAL_1]] : (index) -> !fir.shape<1>
! CHECK: br ^bb1(%[[VAL_3]], %[[VAL_1]] : index, index)
! CHECK: ^bb1(%[[VAL_9:.*]]: index, %[[VAL_10:.*]]: index):
! CHECK: %[[VAL_11:.*]] = arith.cmpi sgt, %[[VAL_10]], %[[VAL_3]] : index
! CHECK: cond_br %[[VAL_11]], ^bb2, ^bb6
! CHECK: ^bb2:
! CHECK: %[[VAL_12:.*]] = arith.addi %[[VAL_9]], %[[VAL_2]] : index
! CHECK: %[[VAL_13:.*]] = fir.array_coor %[[VAL_7]](%[[VAL_8]]) %[[VAL_12]] typeparams %[[VAL_6]]#1 : (!fir.ref<!fir.array<10x!fir.char<1,?>>>, !fir.shape<1>, index, index) -> !fir.ref<!fir.char<1,?>>
! CHECK: %[[VAL_14:.*]] = fir.emboxchar %[[VAL_13]], %[[VAL_6]]#1 : (!fir.ref<!fir.char<1,?>>, index) -> !fir.boxchar<1>
! CHECK: %[[VAL_15:.*]] = fir.convert %[[VAL_6]]#1 : (index) -> i32
! CHECK: %[[VAL_16:.*]] = fir.convert %[[VAL_15]] : (i32) -> index
! CHECK: %[[VAL_17:.*]] = fir.call @llvm.stacksave() : () -> !fir.ref<i8>
! CHECK: %[[VAL_18:.*]] = fir.alloca !fir.char<1,?>(%[[VAL_16]] : index) {bindc_name = ".result"}
! CHECK: %[[VAL_19:.*]] = fir.call @_QMchar_elemPelem_return_char(%[[VAL_18]], %[[VAL_16]], %[[VAL_14]]) : (!fir.ref<!fir.char<1,?>>, index, !fir.boxchar<1>) -> !fir.boxchar<1>
! CHECK: %[[VAL_20:.*]] = arith.cmpi slt, %[[VAL_6]]#1, %[[VAL_16]] : index
! CHECK: %[[VAL_21:.*]] = select %[[VAL_20]], %[[VAL_6]]#1, %[[VAL_16]] : index
! CHECK: %[[VAL_22:.*]] = fir.convert %[[VAL_21]] : (index) -> i64
! CHECK: %[[VAL_23:.*]] = fir.convert %[[VAL_13]] : (!fir.ref<!fir.char<1,?>>) -> !fir.ref<i8>
! CHECK: %[[VAL_24:.*]] = fir.convert %[[VAL_18]] : (!fir.ref<!fir.char<1,?>>) -> !fir.ref<i8>
! CHECK: fir.call @llvm.memmove.p0i8.p0i8.i64(%[[VAL_23]], %[[VAL_24]], %[[VAL_22]], %[[VAL_4]]) : (!fir.ref<i8>, !fir.ref<i8>, i64, i1) -> ()
! CHECK: %[[VAL_25:.*]] = arith.subi %[[VAL_6]]#1, %[[VAL_2]] : index
! CHECK: %[[VAL_26:.*]] = fir.undefined !fir.char<1>
! CHECK: %[[VAL_27:.*]] = fir.insert_value %[[VAL_26]], %[[VAL_5]], [0 : index] : (!fir.char<1>, i8) -> !fir.char<1>
! CHECK: %[[VAL_28:.*]] = arith.subi %[[VAL_25]], %[[VAL_21]] : index
! CHECK: %[[VAL_29:.*]] = arith.addi %[[VAL_28]], %[[VAL_2]] : index
! CHECK: br ^bb3(%[[VAL_21]], %[[VAL_29]] : index, index)
! CHECK: ^bb3(%[[VAL_30:.*]]: index, %[[VAL_31:.*]]: index):
! CHECK: %[[VAL_32:.*]] = arith.cmpi sgt, %[[VAL_31]], %[[VAL_3]] : index
! CHECK: cond_br %[[VAL_32]], ^bb4, ^bb5
! CHECK: ^bb4:
! CHECK: %[[VAL_33:.*]] = fir.convert %[[VAL_13]] : (!fir.ref<!fir.char<1,?>>) -> !fir.ref<!fir.array<?x!fir.char<1>>>
! CHECK: %[[VAL_34:.*]] = fir.coordinate_of %[[VAL_33]], %[[VAL_30]] : (!fir.ref<!fir.array<?x!fir.char<1>>>, index) -> !fir.ref<!fir.char<1>>
! CHECK: fir.store %[[VAL_27]] to %[[VAL_34]] : !fir.ref<!fir.char<1>>
! CHECK: %[[VAL_35:.*]] = arith.addi %[[VAL_30]], %[[VAL_2]] : index
! CHECK: %[[VAL_36:.*]] = arith.subi %[[VAL_31]], %[[VAL_2]] : index
! CHECK: br ^bb3(%[[VAL_35]], %[[VAL_36]] : index, index)
! CHECK: ^bb5:
! CHECK: fir.call @llvm.stackrestore(%[[VAL_17]]) : (!fir.ref<i8>) -> ()
! CHECK: %[[VAL_37:.*]] = arith.subi %[[VAL_10]], %[[VAL_2]] : index
! CHECK: br ^bb1(%[[VAL_12]], %[[VAL_37]] : index, index)
! CHECK: ^bb6:

implicit none
character(*) :: c(10)
Expand Down
Loading

0 comments on commit fb8eb63

Please sign in to comment.