This is an archive of the discontinued LLVM Phabricator instance.

[SimplifyCFG] Find the smallest table considering overflow in `switchToLookupTable`
Changes PlannedPublic

Authored by DianQK on Jul 30 2023, 6:27 AM.

Diff Detail

Event Timeline

DianQK created this revision.Jul 30 2023, 6:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2023, 6:27 AM
DianQK requested review of this revision.Jul 30 2023, 6:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 30 2023, 6:27 AM
DianQK edited the summary of this revision. (Show Details)Jul 30 2023, 6:56 AM
nikic added inline comments.Jul 30 2023, 11:40 AM
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
6436

I don't like that the whole fold gets invoked twice with this flag flipped. Can we just pick the Min/Max that result in the smallest table size (i.e. Max-Min) here?

Ideally we would not even base this on signed/unsigned at all, because the smallest range might be both signed *and* unsigned wrapping. Consider something like -128, -1, 0, 127. This would pick [-128, 127] as the signed range, [0, -1] as the unsigned range, but a minimal range would be [127, 0] or [-1, -128]. This would be based around the largest gap between the case value. Though just signed/unsigned is probably the main useful case.

DianQK retitled this revision from [SimplifyCFG] Add unsigned max-min comparison to `switchToLookupTable`. to [SimplifyCFG] Find the smallest table considering overflow in `switchToLookupTable`.Jul 31 2023, 8:05 AM
DianQK edited the summary of this revision. (Show Details)
DianQK updated this revision to Diff 545675.Jul 31 2023, 8:12 AM
DianQK retitled this revision from [SimplifyCFG] Find the smallest table considering overflow in `switchToLookupTable` to [SimplifyCFG] Find the smallest table considering overflow in `switchToLookupTable`.

Try to find a minimal range.
Some test cases got better and some got worse.

DianQK added a comment.EditedAug 14 2023, 7:59 AM

I need to check these test case changes carefully.

I ran llvm-lit with alive. The result is:

Failed Tests (2):
  LLVM :: Transforms/SimplifyCFG/fold-branch-to-common-dest.ll
  LLVM :: Transforms/SimplifyCFG/speculate-call.ll

I haven't focused on whether this is an issue with alive or in itself, but it doesn't seem to be related to this patch.

I believe @signed_overflow1, @signed_overflow2, and @signed_overflow_negative have all changed for the better. @signed_overflow1 and @signed_overflow_negative exchange results. But since we prefer to start at 0, this reduces the number of sub/add instructions by one.
@covered_switch_with_bit_tests also becomes better, smaller table and fewer commands.
@f_i8_128 is our new test. @_TFO6reduce1E5toRawfS0_FT_Si becomes better.

@f_min_max_2, @f_min_max, @test, and @coveredswitch_test look to become worse. But their table size did get smaller.
I rechecked @f_min_max_2, @f_min_max and @test. They become from table with holes to table without holes. It's a "perfect" table?
I've also found that they luckily have a max and min value. Then we created a cover table.
If we want to deal with this situation, maybe we should add a condition to extend to a cover table.

@coveredswitch_test also luckily have a max and min value.

Maybe ready for a new review.

These test cases that went worse just happened to have the max and min values of bit width, and then created a cover table with holes.
I think I should create another patch that adds a threshold condition to grow into a cover table to handle this.

I'm still not sure about the correctness of this. It seems like only setting the BeginCaseVal and EndCaseVal to the appropriate values is enough to handle overflowing switch cases. Test transformations themselves look correct!

llvm/lib/Transforms/Utils/SimplifyCFG.cpp
6439

I think it's ok to use auto on this.

I'm still not sure about the correctness of this. It seems like only setting the BeginCaseVal and EndCaseVal to the appropriate values is enough to handle overflowing switch cases. Test transformations themselves look correct!

Could you explain more about the overflow?
If you're referring to RangeOverflow in the code, this is a special handling to avoid redundant calculation.
Or do you mean that we can always find the smallest table whose End minus Begin does not overflow? Yes. But we need to introduce an overflow offset calculation.
For example, [122, -128]([122, 128]) in the @f_i8_128 case.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp
6439

Thanks, if Nikita will continue to review it here, I'll change it directly. Otherwise I'll change it when I move to GitHub.

I'm still not sure about the correctness of this. It seems like only setting the BeginCaseVal and EndCaseVal to the appropriate values is enough to handle overflowing switch cases. Test transformations themselves look correct!

I seem to understand what you mean. Yes, this patch is looking for the appropriate BeginCaseVal and EndCaseVal.