Page MenuHomePhabricator

[CallSiteInfo] Handle bundles when updating call site info
ClosedPublic

Authored by djtodoro on Feb 20 2020, 7:58 AM.

Details

Summary

This will address the issue: P8198 and P8199 (from D73534).

The methods was not handle bundles properly.

Diff Detail

Event Timeline

djtodoro created this revision.Feb 20 2020, 7:58 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2020, 7:58 AM

Maybe we can discuss about calling the eraseCallSiteInfo() within MachineFunction::DeleteMachineInstr(), but find all the places where we should call move*()/copy*() call site info.

vsk added inline comments.Feb 20 2020, 3:22 PM
llvm/lib/CodeGen/MachineBasicBlock.cpp
158 ↗(On Diff #245658)

I don't think we should erase call site info in deleteNode. Part of the reason the assert in DeleteMachineInstr was added, IIUC, was to help identify the areas where llvm should move or copy call site info. I fear that if we change deleteNode like this, we'd lose the ability to do that.

Could we address the issue in P8199 by call eraseCallSiteInfo in TargetInstrInfo::ReplaceTailWithBranchTo instead?

llvm/lib/CodeGen/MachineFunction.cpp
396

This part lgtm.

djtodoro marked an inline comment as done.Feb 20 2020, 5:08 PM
djtodoro added inline comments.
llvm/lib/CodeGen/MachineBasicBlock.cpp
158 ↗(On Diff #245658)

The eraseCallSiteInfo() was not called within the ReplaceTailWithBranchTo() (but there is a piece of code for that), since we started erasing an instruction that is not the candidate for call site entry, and then when we started erasing the instruction (actually the instr range starting from the MI) with erase(), a call site candidate occurred. So, that is the reason I put that within deleteNode() (there were no other functions called after the ReplaceTailWithBranchTo()).

I agree that we break the idea with the assert, but it might be more acceptable to put an assertion in CloneMachineInstr() and places like that, rather than in DeleteMachineInstr()?

vsk added a comment.Feb 20 2020, 5:19 PM

At this point, what I think we should do is:

  1. Disable the assert in DeleteMachineInstr and file a PR to re-enable it.
  2. Land D73534.
  3. Once we're sure D73534 has "stuck" in tree, iterate to re-enable the assert.

I don't see a downside to that approach, and it'll be beneficial for downstream targets to have D73534 landed for good.

llvm/lib/CodeGen/MachineBasicBlock.cpp
158 ↗(On Diff #245658)

I see something slightly different.

ReplaceTailWithBranchTo does already call eraseCallSiteInfo, but eraseCallSiteInfo doesn't appear to handle bundles:

frame #4: 0x00000001012fde01 clang-10`llvm::MachineFunction::DeleteMachineInstr(this=0x0000000107b67420, MI=0x00000001089620d0) at MachineFunction.cpp:386:3
   383    // be triggered during the implementation of support for the
   384    // call site info of a new architecture. If the assertion is triggered,
   385    // back trace will tell where to insert a call to updateCallSiteInfo().
-> 386    assert((!MI->isCandidateForCallSiteEntry() ||
   387            CallSitesInfo.find(MI) == CallSitesInfo.end()) &&
   388           "Call site info was not updated!");
   389    // Strip it for parts. The operand array and the MI object itself are
(lldb) p MI->dump()
  UNKNOWN 1, killed $physreg3, @_ZdlPv, <regmask ...>, implicit-def dead $physreg13, implicit $physreg15, implicit internal killed $physreg72, implicit-def $physreg15, implicit internal killed $physreg72, implicit-def dead $physreg72, implicit internal killed $physreg12
(lldb) p MI->isCandidateForCallSiteEntry()
(bool) $0 = true

(lldb) up
frame #9: 0x00000001015154f8 clang-10`llvm::TargetInstrInfo::ReplaceTailWithBranchTo(this=<unavailable>, Tail=<unavailable>, NewDest=0x000000010894ff28) const at TargetInstrInfo.cpp:147:10 [opt]
   144      auto MI = Tail++;
   145      if (MI->isCandidateForCallSiteEntry())
   146        MBB->getParent()->eraseCallSiteInfo(&*MI);
-> 147      MBB->erase(MI);
djtodoro marked an inline comment as done.Feb 20 2020, 10:25 PM

I don't see a downside to that approach, and it'll be beneficial for downstream targets to have D73534 landed for good.

Strong +1

llvm/lib/CodeGen/MachineBasicBlock.cpp
158 ↗(On Diff #245658)

Yes, the bundle is the problem.

Hm, I agree, I should just add handling of bundles within eraseCallSiteInfo(), that will handle this. That is better idea.

I was occupied with the idea of removing the assert, and did not think about that alternative. I’ll update the patch :)

djtodoro marked an inline comment as done.Feb 20 2020, 10:30 PM
djtodoro added inline comments.
llvm/lib/CodeGen/MachineBasicBlock.cpp
158 ↗(On Diff #245658)

Or the isCandidateForCallSite() should better handle bundles. I think it was 0 in the ‘up’ frame.

djtodoro marked an inline comment as done.Feb 25 2020, 7:38 AM
djtodoro added inline comments.
llvm/lib/CodeGen/MachineBasicBlock.cpp
158 ↗(On Diff #245658)

The problem is the MI (bundle) from the up frame was not marked as candidate for call site entry, but within that bundle was a call (candidate), and when we started erasing the MI, we found the call for which we had not updated the call site info (and it was too late, since the erase() for instr range started from the bundle MI which was not marked as a candidate).

So, we should handle bundles on both sides (isCandidateForCallSite() and methods for updating the call site info).

Examples

(mips)

CALL {
  instr1
}

(arm thumb)

BUNDLE {
  instr1
  instr2
  call_instr
}
djtodoro updated this revision to Diff 246462.Feb 25 2020, 7:43 AM
djtodoro retitled this revision from WIP: [CallSiteInfo] Erase call site info for a MI from bundle to [CallSiteInfo] Handle bundles when updating call site info.
djtodoro edited the summary of this revision. (Show Details)

-Handle bundles within:

  • isCandidateForCallSiteEntry()
  • erase, copy and move call site methods

TODO: Reduce the test case.

dstenb added inline comments.Feb 25 2020, 8:37 AM
llvm/lib/CodeGen/MachineFunction.cpp
886

As the {erase,copy,move} functions seems to handle bundles in the same way, perhaps it is worth breaking this out to a common helper that either returns MI if it's not a bundle, or finds the call instruction in the bundle?

djtodoro marked an inline comment as done.Feb 26 2020, 2:21 AM
djtodoro added inline comments.
llvm/lib/CodeGen/MachineFunction.cpp
886

Makes sense, thanks.

djtodoro updated this revision to Diff 246654.Feb 26 2020, 2:23 AM

-Addressing comments
-Reducing the test case

I see this was triggered in a case when we have not used the '-g' option at all.

When we started the work, there were thoughts the "call-site-info" could be used somewhere else, but it is not the case now. It is being used only for the Debug Entry Values feature, so I think it makes sense to avoid production (and handling though out the pipeline) of the call-site-info if not '-g' and '-O>0'.

The DWARF generation (for call-site-params) is controlled the same way as production of call-sites by looking at the DISupprogam flag AllCallsDescribed.

I see this was triggered in a case when we have not used the '-g' option at all.

When we started the work, there were thoughts the "call-site-info" could be used somewhere else, but it is not the case now. It is being used only for the Debug Entry Values feature, so I think it makes sense to avoid production (and handling though out the pipeline) of the call-site-info if not '-g' and '-O>0'.

The DWARF generation (for call-site-params) is controlled the same way as production of call-sites by looking at the DISupprogam flag AllCallsDescribed.

Something like D75175 (and then rebase reland of the D73534 on top of that).

I see this was triggered in a case when we have not used the '-g' option at all.

When we started the work, there were thoughts the "call-site-info" could be used somewhere else, but it is not the case now. It is being used only for the Debug Entry Values feature, so I think it makes sense to avoid production (and handling though out the pipeline) of the call-site-info if not '-g' and '-O>0'.

The DWARF generation (for call-site-params) is controlled the same way as production of call-sites by looking at the DISupprogam flag AllCallsDescribed.

Something like D75175 (and then rebase reland of the D73534 on top of that).

Does that patch make this one redundant, or can we land in the cases that this patch fixes with -O>0?

llvm/lib/CodeGen/MachineFunction.cpp
879–882

Sorry, I missed this the first round, but perhaps it's worth making this a range-based loop?

for (auto &BMI : make_range(getBundleStart(MI->getIterator()),
                            getBundleEnd(MI->getIterator())))
vsk accepted this revision.Feb 26 2020, 8:55 AM

lgtm with a nit, and @dstenb's comment addressed.

llvm/include/llvm/CodeGen/MachineInstr.h
690

A better description might be: "Returns true if copying, moving, or erasing this instruction requires updating Call Site Info (see \ref copyCallSiteInfo, \ref moveCallSiteInfo, etc)."

This revision is now accepted and ready to land.Feb 26 2020, 8:55 AM
djtodoro marked 2 inline comments as done.Feb 27 2020, 1:09 AM

I see this was triggered in a case when we have not used the '-g' option at all.

When we started the work, there were thoughts the "call-site-info" could be used somewhere else, but it is not the case now. It is being used only for the Debug Entry Values feature, so I think it makes sense to avoid production (and handling though out the pipeline) of the call-site-info if not '-g' and '-O>0'.

The DWARF generation (for call-site-params) is controlled the same way as production of call-sites by looking at the DISupprogam flag AllCallsDescribed.

Something like D75175 (and then rebase reland of the D73534 on top of that).

Does that patch make this one redundant, or can we land in the cases that this patch fixes with -O>0?

This patch fixes the problem and we need it for sure. But, the D75175 is another idea (nothing to do with this patch) to avoid production of call site info in the case of non-debug builds, which makes sense to me, since it is being used only for Debug Entry Values feature, but we can move the discussion to that patch.

llvm/include/llvm/CodeGen/MachineInstr.h
690

+1
Thanks!

llvm/lib/CodeGen/MachineFunction.cpp
879–882

Sounds better.

djtodoro updated this revision to Diff 246887.Feb 27 2020, 1:11 AM

-Addressing comments

@dstenb please let me know if this looks good now.

dstenb accepted this revision.Feb 27 2020, 1:18 AM

Does that patch make this one redundant, or can we land in the cases that this patch fixes with -O>0?

This patch fixes the problem and we need it for sure. But, the D75175 is another idea (nothing to do with this patch) to avoid production of call site info in the case of non-debug builds, which makes sense to me, since it is being used only for Debug Entry Values feature, but we can move the discussion to that patch.

Okay, thanks for the explanation!

aprantl added inline comments.Feb 27 2020, 8:49 AM
llvm/lib/CodeGen/BranchFolding.cpp
173

drive-by comment: Should there be a

MachineInstruction::eraseStaleCallSiteInfo() {
  if (shouldUpdateCallSiteInfo())
    getFunction()->eraseCallSiteInfo(&MI);
}

helper?

djtodoro marked an inline comment as done.Feb 27 2020, 11:50 PM
djtodoro added inline comments.
llvm/lib/CodeGen/BranchFolding.cpp
173

I think that makes sense, that can go as an NFC?