Page MenuHomePhabricator

[CorrelatedValuePropagation] Fix prof branch_weights metadata handling for SwitchInst
ClosedPublic

Authored by yrouban on May 20 2019, 1:38 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

yrouban created this revision.May 20 2019, 1:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2019, 1:38 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
nikic added inline comments.May 25 2019, 10:13 AM
llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
398 ↗(On Diff #200218)

I think that something can go wrong here... Consider a case where we have a switch with branch weights, then we remove the first case (so that Changed=true in SwitchInstProfBranchWeightsWrapper), then we find that the second case is always true, such that this constant folding call will erase the switch, leaving behind a dangling reference in SwitchInstProfBranchWeightsWrapper, which will be accessed when the destructor runs at the end of the function.

yrouban marked an inline comment as done.May 27 2019, 7:16 AM
yrouban added inline comments.
llvm/lib/Transforms/Scalar/CorrelatedValuePropagation.cpp
398 ↗(On Diff #200218)

Good catch!
ConstantFoldTerminator() can change SwitchInst on its own.

yrouban updated this revision to Diff 201533.May 27 2019, 7:36 AM
yrouban edited the summary of this revision. (Show Details)
  • renamed SwitchInstProfBranchWeightsWrapper to SwitchInstProfUpdateWrapper
  • explicitly narrowed scope of the SwitchInstProfUpdateWrapper object so it does not overlap with ConstantFoldTerminator()
  • added a test case where switch turns into conditional branch
nikic added a comment.May 27 2019, 1:57 PM

This looks ok, but I wonder if it might make sense to have an explicit update() method for cases (like this) where we need to control the point where the SwitchInst is updated. Something like:

void update() {
  if (Changed)
    SI.setMetadata(LLVMContext::MD_prof, buildProfBranchWeightsMD());
  Changed = false;
}
~SwitchInstProfUpdateWrapper() {
  update();
}

I feel like this might be more obvious than extra scopes to control the destruction point.

I considered this way and found it error prone. The wrapper stays alive after ConstantFoldTerminator() and developer may accidentally reuse it but the stored weights might be unsync with the underlying SwitchInst's weights changed by ConstantFoldTerminator().

nikic accepted this revision.May 28 2019, 12:18 AM

LGTM

llvm/test/Transforms/CorrelatedValuePropagation/profmd.ll
49 ↗(On Diff #201533)

These check lines should probably be positioned one block higher?

This revision is now accepted and ready to land.May 28 2019, 12:18 AM
yrouban marked an inline comment as done.May 28 2019, 3:21 AM
This revision was automatically updated to reflect the committed changes.

FYI, we're seeing a crash in llvm::SwitchInstProfUpdateWrapper::getProfBranchWeights() following this patch.
I'm trying to get a testcase available.

nikic added a comment.May 28 2019, 2:13 PM

My guess would be that this is due to the assumption of valid branch_weights inside https://github.com/llvm-mirror/llvm/blob/2015c9266833ebae0e42a641252b2b120ef0f77a/lib/IR/Instructions.cpp#L3908, which is not a guarantee we have at this point.

nikic added a comment.May 28 2019, 2:21 PM

Providing an obviously incorrect branch_weights for one of the added test cases is enough to cause an assertion failure:

define i32 @crash(i32 %s) {
entry:
  %cmp = icmp sgt i32 %s, 0
  br i1 %cmp, label %positive, label %out

positive:
  switch i32 %s, label %out [
  i32 1, label %next
  i32 -1, label %next
  i32 -2, label %next
  ], !prof !{!"branch_weights"} ; not enough elements

out:
  %p = phi i32 [ -1, %entry ], [ 1, %positive ]
  ret i32 %p

next:
  %q = phi i32 [ 0, %positive ], [ 0, %positive ], [ 0, %positive ]
  ret i32 %q
}
nikic reopened this revision.May 28 2019, 2:28 PM

Reverted with rL361881 for now.

This revision is now accepted and ready to land.May 28 2019, 2:28 PM

Thank you for the quick action!

yrouban planned changes to this revision.May 29 2019, 4:33 AM

Thank you very much for the revert, the analysis and the test.
I'm going make the wrapper class safe by introducing invalid sticky state.

yrouban requested review of this revision.May 30 2019, 6:08 AM

Providing an obviously incorrect branch_weights for one of the added test cases is enough to cause an assertion failure:

I have just submitted a fix. Please see D62656. Once it is landed this patch can be landed unchanged.
I decided to not include the crash test case as it would be caught by verification proposed in D61179.

This revision is now accepted and ready to land.May 30 2019, 6:08 AM
This revision was automatically updated to reflect the committed changes.