[PATCH] D145008: [ControlHeightReduction] Don't combine a "poison" branch

[David Li via Phabricator via llvm-commits]

davidxl added a comment.

In D145008#4162517 <https://reviews.llvm.org/D145008#4162517>, @nikic wrote:

> In D145008#4162106 <https://reviews.llvm.org/D145008#4162106>, @davidxl wrote:
>
>> In D145008#4160793 <https://reviews.llvm.org/D145008#4160793>, @nikic wrote:
>>
>>> The value may be poison even if it's not literally poison. The problem seems to be that the condition gets swapped. CHR generates `select i1 poison, i1 %arg, i1 false` rather than the correct `select i1 %arg, i1 poison, i1 false`. Either that swapping should not occur (if it's not important to what CHR is doing) or the condition needs to be frozen.
>>
>> Correct -- kazu@ brought it up before sending the patch as well -- is there existing interface to query for propagated poison?
>
> There is `isGuaranteedNotToBeUndefOrPoison()`, but here you want to use `freeze` as a fallback.
>
>> The swap is triggered because of the profile data says it is profitable -- while it is itself a bug to deal with, it is independent to this fix.
>
> How can the swap be profitable when flattening control flow into a select? Shouldn't it be independent of order at that point?

It looks like there is a hidden profit analysis issue to be fixed, shown in the modified test below.   The first branch does not have profile data attached, but CHR still decides to swap the order of the combined branch (from BB3 and BB5) with it.
@n = dso_local local_unnamed_addr global i32 0, align 4

define void @chr_poison(i1 %arg, i1 %arg2, i1 %arg3) !prof !29 {

  br i1 %arg2, label %bb3, label %bb2

bb2:

  store i32 2, ptr @n, align 4
  ret void

bb3:

  store i32 3, ptr @n, align 4
  br i1 %arg3, label %bb5, label %bb4, !prof !30

bb4:

  store i32 4, ptr @n, align 4
  br label %bb5

bb5:

  store i32 5, ptr @n, align 4
  br i1 %arg, label %bb2, label %bb6, !prof !31

bb6:

  store i32 6, ptr @n, align 4
  br label %bb2

bb7:

  store i32 7, ptr @n, align 4
  br label %bb5

}

!llvm.module.flags = !{!0}

!0 = !{i32 1, !"ProfileSummary", !1}
!1 = !{!2, !3, !4, !5, !6, !7, !8, !9, !10, !11}
!2 = !{!"ProfileFormat", !"SampleProfile"}
!3 = !{!"TotalCount", i64 2625732223}
!4 = !{!"MaxCount", i64 6099111}
!5 = !{!"MaxInternalCount", i64 0}
!6 = !{!"MaxFunctionCount", i64 8812263}
!7 = !{!"NumCounts", i64 1399554}
!8 = !{!"NumFunctions", i64 14940}
!9 = !{!"IsPartialProfile", i64 0}
!10 = !{!"PartialProfileRatio", double 0.000000e+00}
!11 = !{!"DetailedSummary", !12}
!12 = !{!13, !14, !15, !16, !17, !18, !19, !20, !21, !22, !23, !24, !25, !26, !27, !28}
!13 = !{i32 10000, i64 6099103, i32 8}
!14 = !{i32 100000, i64 6083920, i32 44}
!15 = !{i32 200000, i64 5615450, i32 92}
!16 = !{i32 300000, i64 658615, i32 301}
!17 = !{i32 400000, i64 276706, i32 1049}
!18 = !{i32 500000, i64 224143, i32 2110}
!19 = !{i32 600000, i64 155027, i32 3480}
!20 = !{i32 700000, i64 62158, i32 6258}
!21 = !{i32 800000, i64 29875, i32 12602}
!22 = !{i32 900000, i64 5684, i32 34979}
!23 = !{i32 950000, i64 1439, i32 81728}
!24 = !{i32 990000, i64 319, i32 227620}
!25 = !{i32 999000, i64 34, i32 416158}
!26 = !{i32 999900, i64 4, i32 603970}
!27 = !{i32 999990, i64 1, i32 750171}
!28 = !{i32 999999, i64 1, i32 750171}
!29 = !{!"function_entry_count", i64 3204}
!30 = !{!"branch_weights", i32 2126980204, i32 20503444}
!31 = !{!"branch_weights", i32 997, i32 2}


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145008