[PATCH] D154579: [InstCombine] Only perform one iteration

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

Left some comments on test diffs. I don't think any of the remaining cases are particularly problematic, though the phi and freeze cases are something that may be worth fixing.



================
Comment at: llvm/test/Analysis/ValueTracking/numsignbits-from-assume.ll:51
 ; CHECK-LABEL: @computeNumSignBits_sub2(
-; CHECK-NEXT:    [[SUB:%.*]] = add nsw i32 [[IN:%.*]], -1
+; CHECK-NEXT:    [[SUB:%.*]] = add i32 [[IN:%.*]], -1
 ; CHECK-NEXT:    [[COND:%.*]] = icmp ult i32 [[SUB]], 43
----------------
This is related to backwards-propagation of assumes: Assumes can affect guaranteed-to-transfer instructions in a limited window before the assume. We may fail to fold such cases in one iteration if we first need to fold instructions to bring the assume into a recognized form. Here the assume is only recognized by AC after ule is converted to ult, at which point the add before has already been visited.

I don't think this issue matters in practice.


================
Comment at: llvm/test/Transforms/InstCombine/merging-multiple-stores-into-successor.ll:31
 ; CHECK-NEXT:    [[STOREMERGE1:%.*]] = phi i32 [ [[I11]], [[BB10]] ], [ 1, [[BB9]] ]
+; CHECK-NEXT:    [[STOREMERGE:%.*]] = phi i32 [ 1, [[BB9]] ], [ [[I11]], [[BB10]] ]
 ; CHECK-NEXT:    store i32 [[STOREMERGE1]], ptr @arr_2, align 4
----------------
This is caused by details of how we canonicalize phi operand order. This is easy to fix, it just has annoying test fallout.


================
Comment at: llvm/test/Transforms/InstCombine/pr55228.ll:11
 ; CHECK-LABEL: @test(
-; CHECK-NEXT:    [[CMP:%.*]] = icmp eq ptr [[P:%.*]], getelementptr inbounds (i8, ptr @g, i8 1)
+; CHECK-NEXT:    [[CMP:%.*]] = icmp eq ptr [[P:%.*]], getelementptr inbounds (i8, ptr @g, i64 1)
 ; CHECK-NEXT:    ret i1 [[CMP]]
----------------
This happens because the initializer of the global is not fully folded. This is not a problem when run in a real optimization pipeline, because GlobalOpt will handle such cases earlier.


================
Comment at: llvm/test/Transforms/InstCombine/shift.ll:1718
 ; CHECK-NEXT:    [[C171:%.*]] = icmp slt i177 [[L7_FROZEN]], 0
-; CHECK-NEXT:    [[C17:%.*]] = and i1 [[TMP1]], [[C171]]
+; CHECK-NEXT:    [[C17:%.*]] = select i1 [[TMP1]], i1 [[C171]], i1 false
 ; CHECK-NEXT:    [[TMP3:%.*]] = sext i1 [[C17]] to i64
----------------
I didn't bother looking into this, because it's a fuzzer test case.


================
Comment at: llvm/test/Transforms/PGOProfile/chr.ll:1936
+; CHECK-NEXT:    [[CMP0:%.*]] = icmp ne i64 [[J_FR]], [[K:%.*]]
+; CHECK-NEXT:    [[TMP0:%.*]] = freeze i1 [[CMP0]]
 ; CHECK-NEXT:    [[CMP3:%.*]] = icmp ne i64 [[I_FR]], [[J_FR]]
----------------
At the time we process this freeze, j.fr hasn't been introduced yet, so we would have to introduce two freeze instructions. We could fix this by allowing the creation of more than one freeze when pushing upward. Especially for icmps that is probably beneficial.


================
Comment at: llvm/test/Transforms/PhaseOrdering/AArch64/matrix-extract-insert.ll:119
+; CHECK-NEXT:    [[CONV_US_1:%.*]] = zext i32 [[K_011_US_1]] to i64
+; CHECK-NEXT:    [[TMP8:%.*]] = add nuw nsw i64 [[CONV_US_1]], 15
 ; CHECK-NEXT:    [[TMP9:%.*]] = icmp ult i32 [[K_011_US_1]], 210
----------------
This is the same backwards reasoning assume issue mentioned above.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D154579/new/

https://reviews.llvm.org/D154579