Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug in HeapToStack optimization pass #4341

Merged
merged 4 commits into from Apr 11, 2023
Merged

Fix bug in HeapToStack optimization pass #4341

merged 4 commits into from Apr 11, 2023

Conversation

SeanTAllen
Copy link
Member

@SeanTAllen SeanTAllen commented Apr 1, 2023

Fix bug in HeapToStack optimization

Our HeapToStack optimization pass was occassionally generating incorrect IR.

The following is the "annotated by Pony developers" IR after a HeapToStack
optimization pass run for the code from Issue #4340:

define private fastcc ptr @_TestUnivals_ref_apply_oo(ptr nocapture readnone %this, ptr nocapture readonly dereferenceable(24) %h) unnamed_addr !dbg !7533 !pony.abi !4 {
entry:
  %_leaf_right = alloca i8, i64 32, align 8
  %_leaf_left = alloca i8, i64 32, align 8
  store ptr @Leaf_Desc, ptr %_leaf_left, align 8
  %_leaf_left_value = getelementptr inbounds %Leaf, ptr %_leaf_left, i64 0, i32 1, !dbg !7539
  store i64 0, ptr %_leaf_left_value, align 8, !dbg !7539
  store ptr @Leaf_Desc, ptr %_leaf_right, align 8
  %_leaf_right_value = getelementptr inbounds %Leaf, ptr %_leaf_right, i64 0, i32 1, !dbg !7542
  store i64 0, ptr %_leaf_right_value, align 8, !dbg !7542
  %_leaf_left_univals = tail call fastcc i64 @Leaf_ref_univals_Z(ptr nonnull %_leaf_left), !dbg !7547
  %_leaf_left_get_value = tail call fastcc i64 @Leaf_ref_get_value_Z(ptr nonnull %_leaf_left), !dbg !7548
  %_leaf_right_get_value = tail call fastcc i64 @Leaf_ref_get_value_Z(ptr nonnull %_leaf_right), !dbg !7549
  %8 = icmp eq i64 %_leaf_left_get_value, %_leaf_right_get_value
  br i1 %8, label %sc_right.i, label %Node_ref_univals_Z.exit

sc_right.i:                                       ; preds = %entry
  %9 = icmp eq i64 %_leaf_left_get_value, 0
  %10 = zext i1 %9 to i64
  %spec.select.i = add i64 %_leaf_left_univals, %10
  br label %Node_ref_univals_Z.exit

Node_ref_univals_Z.exit:                          ; preds = %entry, %sc_right.i
  %.pn.i = phi i64 [ %_leaf_left_univals, %entry ], [ %spec.select.i, %sc_right.i ]
  %_leaf_right_univals = tail call fastcc i64 @Leaf_ref_univals_Z(ptr nonnull %_leaf_right), !dbg !7550
  %12 = add i64 %.pn.i, %_leaf_right_univals
  %13 = tail call fastcc i1 @pony_test_TestHelper_val__check_eq_USize_val_oZZoob(ptr nonnull %h, ptr nonnull @19, i64 3, i64 %12, ptr nonnull @39, ptr nonnull @"$1$0_Inst"), !dbg !7555
  call void @llvm.dbg.value(metadata ptr @None_Inst, metadata !5286, metadata !DIExpression()), !dbg !7556
  ret ptr @None_Inst, !dbg !7558
}

The calls such as:

  %_leaf_left_get_value = tail call fastcc i64 @Leaf_ref_get_value_Z(ptr nonnull %_leaf_left), !dbg !7548

are incorrect because, tail indicates that the function in question won't
touch memory from any alloca. However, our HeapToStack pass was being too
simplistic in its analysis and not setting all calls to be "tail false" that
touches memory that comes from an alloca.

In the above example, you can see that @Leaf_ref_get_value_Z uses
tales a pointer to the %_leaf_left alloca but because it is marked as "tail",
later optimizations like dead store elimination will see:

  %_leaf_left_value = getelementptr inbounds %Leaf, ptr %_leaf_left, i64 0, i32 1, !dbg !7539
  store i64 0, ptr %_leaf_left_value, align 8, !dbg !7539

as dead code and remove the initialization of the object in question. From
there, we get incorrect code.

The problem arose because the HeapToStack pass wasn't doing any sort of alias
analysis when looking for functions within a function where we converted a
heap allocation to a stack one. This would cause it to incorrectly believe that
some functions that where marked as "tail" didn't need to be changed to
"not tail".

This commit, instead of adding a clever alias analysis step instead sets all
calls within a function to "not tail" if we add an alloca to as part of a
heap to stack optimization.

Fixes #4340

@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Apr 1, 2023
@SeanTAllen
Copy link
Member Author

Opening this with no fix, just a regression test that we should see fail on Arm.

@SeanTAllen
Copy link
Member Author

Excellent our regression correctly fails in CI where expected. This means we can move forward with a fix once one is agreed upon.

@SeanTAllen
Copy link
Member Author

Interesting. We have x86 failing for this regression as well. I wasn't able to get it to fail locally on x86 but, there ya go! This failing on all platforms in CI is a much better result as that makes more sense.

@SeanTAllen SeanTAllen changed the title Fix weird optimization created Arm bug Fix optimization interaction bug Apr 1, 2023
@SeanTAllen SeanTAllen removed the discuss during sync Should be discussed during an upcoming sync label Apr 4, 2023
@SeanTAllen SeanTAllen requested a review from jemc April 11, 2023 22:10
@ponylang-main ponylang-main added the discuss during sync Should be discussed during an upcoming sync label Apr 11, 2023
@SeanTAllen SeanTAllen marked this pull request as ready for review April 11, 2023 22:10
@SeanTAllen SeanTAllen changed the title Fix optimization interaction bug Fix bug in HeapToStack optimization pass Apr 11, 2023
@SeanTAllen SeanTAllen added the changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge label Apr 11, 2023
@SeanTAllen SeanTAllen merged commit 7caadd5 into main Apr 11, 2023
21 checks passed
@SeanTAllen SeanTAllen deleted the issue-4340 branch April 11, 2023 23:08
@ponylang-main ponylang-main removed the discuss during sync Should be discussed during an upcoming sync label Apr 11, 2023
github-actions bot pushed a commit that referenced this pull request Apr 11, 2023
github-actions bot pushed a commit that referenced this pull request Apr 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect program result when not using --debug
3 participants