Page MenuHomePhabricator

[ISEL][X86] Tracking of registers that forward call arguments
ClosedPublic

Authored by djtodoro on Apr 15 2019, 8:47 AM.

Details

Summary

While lowering calls collect info about registers that forward arguments into following function frame. We store such info into MachineFunction of the call.
This is used very late when dumping DWARF info about call site parameters.

NOTE: Other architectures should follow similar procedure buy collecting call argument forwarding register in theirs LowerCallspecialization. Once the call site info is collected it should be bound to SDNode that represents the call instruction. Backend support should keep track about updating MachineInstr call instructions. For more info about backend call site support see https://reviews.llvm.org/D61062.

Authors: @asowda, @NikolaPrica, @djtodoro, @ivanbaev

Diff Detail

Event Timeline

djtodoro created this revision.Apr 15 2019, 8:47 AM

Looking through the file list, this seems to be missing FastISel and GlobalIsel support?

include/llvm/CodeGen/MIRYamlMapping.h
340 ↗(On Diff #195196)

This could use some Doxygen comments :-)

bjope added a subscriber: bjope.Apr 17 2019, 12:18 AM

Looks generally good. But...

I think the subject and summary of this patch is a little bit misleading. It talks about ISEL and lowering of calls, but that only seems to be a small part of it.

Afaict this patch only implements the "full" call lowering support for X86, right? What should happen for other targets when using -emit-call-site-info (afaict SelectionDAG still would generate some callsite information)?
Should probably elaborate a little bit about only supporting X86 and what other targets would need to do.

A substantial part of this patch is touching Codegen in a more general way to support storing the callsite info in the MachineFunction, and serializing that information in the MIR format. Not really related to ISEL IMHO :-)
Should perhaps MIRLangReg.rst be updated in this patch to describe the new additions (or is that covered by another patch in the stack)?.

The patch is also updating passes like InlineSpiller, PeepholeOptimizer and MachineOutliner (this is not about ISEL either) to support keeping the callsite info valid and up-to-date through the backend.
I guess this is putting some new requirements on all backend passes. It would be good if the summary (commit msg) would explain a little bit more about the implications with the solution (such as when MF.updateCallSiteInfo(...); should be used). If I for example want to use this functionality in my OOT target such information is extra helpful, as I need to make all such patches myself. But such information could also be useful for in-tree development, e.g. for reviewers to understand if there are more places to update that you have missed (or for other patches that would need to rebase onto this patch later).

lib/CodeGen/TargetInstrInfo.cpp
138 ↗(On Diff #195196)

Not sure if it is worth the complexity to mix the update of call site info and erasing in the same loop here? Always tricky to remove elements while iterating over the same data structure (even if I think you got it correct here).

test/CodeGen/X86/call-site-info-output.ll
8

Afaict offset: 14 just matches what we currently get (so it is not important that we get 14 here). We should check for offset: {{[0-9]+}} instead, to make the testcase less sensitive to future unrelated changes in lowering, right?

12

This verifies that the parser accepts the new syntax. But not really that it does anything useful with it (it could just discard it and this test would still pass). Maybe that is supposed to be tested elsewhere? Otherwise one idea could be to stop before expand-isel-pseudos in the RUN above, then run expand-isel-pseudos and check that we still get the same callSites info in the output, and then start-after expand-isel-pseudos for the last part of the testing.

@bjope @aprantl Thank you for your comments!

Looking through the file list, this seems to be missing FastISel and GlobalIsel support?

Yes. We taught that it might go as a separate patch? Or should we add it all together?

Firstly this patch was in two parts but for the first review we left it as this in order to see big picture. We will separate it on ISEL part and backend handling part for sure!

Should probably elaborate a little bit about only supporting X86 and what other targets would need to do.

We will do that.

A substantial part of this patch is touching Codegen in a more general way to support storing the callsite info in the MachineFunction, and serializing that information in the MIR format. Not really related to ISEL IMHO :-)
Should perhaps MIRLangReg.rst be updated in this patch to describe the new additions (or is that covered by another patch in the stack)?.

You are right about this but since we produce such data at ISEL phase we saw it as whole. We provide it like that in order to be able to provide tests that we actually changed something in MachineFunction. Like this we can add test that MIR could be parsed by reading .mir produced from test file. But we can separate it on general part that can read given MIR format and ISEL part for X86 that would test writing MIR? Do you think that it will look better like that?

include/llvm/CodeGen/MIRYamlMapping.h
340 ↗(On Diff #195196)

Oh sorry about that. Will add those!

lib/CodeGen/TargetInstrInfo.cpp
138 ↗(On Diff #195196)

This is good question since we have some doubts about this. All updateCallSiteInfo(MI) are used to inform MachineFunction.CallSitesInfo that referenced call instruction got deleted and that it needs update. Now such update can be moved to MachineFunction::DeleteMachineInstr to preserve valid state of CallSiteInfo but we are not quite sure about it since it is frequently used function.
There we left assertion (it should be warped by NDEBUG or removed) that is tested for X86 but will definitely be useful for adding call site info support for other architectures. That assertion brought us to fix this function.
General question is should we move preservation of call site information state to MachineFunction::DeleteMachineInstr or just use assertion in it to point us where should we insert updateCallSiteInfo(MI); ?

test/CodeGen/X86/call-site-info-output.ll
12

Yes, you are right. This only verifies new syntax. Usage of this information is tested in the latest patch. Here we test output for call-site. With new RUN we can test parsing of new syntax and by so there will be no need to test "offset" since parser will verify that information. I will add this as comment for the test.

aprantl added inline comments.Apr 22 2019, 10:02 AM
include/llvm/CodeGen/MachineFunction.h
381 ↗(On Diff #196054)

How about using a struct with field names instead of a std::pair?

955 ↗(On Diff #196054)

/p -> \p

include/llvm/CodeGen/SelectionDAG.h
1634

This function appears to reimplement the operator[] of DenseMap?
I guess it doesn't insert the value...

lib/CodeGen/MIRPrinter.cpp
476 ↗(On Diff #196054)

Can we replace this bodyless loop with something from std::algorithm?

lib/CodeGen/MachineFunction.cpp
365 ↗(On Diff #196054)

assert (!MI->isCall(MachineInstr::IgnoreBundle) || ...)

lib/CodeGen/MachineVerifier.cpp
2150 ↗(On Diff #196054)

typo

lib/CodeGen/TargetInstrInfo.cpp
140 ↗(On Diff #196054)

clang-format

145 ↗(On Diff #196054)

I'd prefer writing two loops (or even better two std::algorithms) for updating and erasing and let the optimizer merge them.

lib/CodeGen/XRayInstrumentation.cpp
114 ↗(On Diff #196054)

So how do we make sure that people will call this in their new passes form here on? Can we verify it?

Thanks for the comments! I've addressed most of them. This patch will be split on 3 parts once we decide how to address comment about updating call site in TargetInstrInfo.

lib/CodeGen/TargetInstrInfo.cpp
145 ↗(On Diff #196054)

I've tried with std::remove_if and llvm::remove_if but they require deleted MachineInstr copy assignment operator. So I came up with two solutions:

Solution 1

// Update call site info and remove all the dead instructions
 // from the end of MBB.
 while (Tail != MBB->end()) {
   auto MI = Tail++;
   if (MI->isCall())
     MBB->getParent()->updateCallSiteInfo(&*MI);
   MBB->erase(MI);
 }

Or

Solution 2

// Update call site info.
 for (auto It = Tail; It != MBB->end(); ++It)
   if (It->isCall())
     MBB->getParent()->updateCallSiteInfo(&*It)

 // Remove all the dead instructions from the end of MBB.
 for (auto It = Tail; It != MBB->end();) {
   auto MI = It++;
   MBB->erase(MI);
 }

@aprantl First example seams better than existing one. I'm wondering whether second with two loops is what you aimed to?

bjope added inline comments.Apr 23 2019, 1:37 PM
lib/CodeGen/TargetInstrInfo.cpp
145 ↗(On Diff #196054)

The second loop in "solution 2" can be reduced to MBB->erase(Tail, MBB->end()); (that is what it used to look like). No need to complicate that part ;-)

bjope added inline comments.Apr 23 2019, 1:42 PM
lib/CodeGen/MIRPrinter.cpp
490 ↗(On Diff #196054)

From CodingStandards.rst:

As a rule of thumb, always make sure to use llvm::sort instead of std::sort.
NikolaPrica added inline comments.Apr 24 2019, 1:44 AM
lib/CodeGen/MIRPrinter.cpp
490 ↗(On Diff #196054)

Thanks for pointing that!

lib/CodeGen/TargetInstrInfo.cpp
145 ↗(On Diff #196054)

MBB->erase(Tail, MBB->end()); recursively deletes elements. I taught that Adrian aimed to usage of two loops because optimizer will be able to merge them. Not sure whether it will be able to merge loop and recursive deletion call.

djtodoro updated this revision to Diff 196428.Apr 24 2019, 5:30 AM
djtodoro retitled this revision from [ISEL] Collect argument's forwarding regs when lowering calls to [ISEL][X86] Tracking of registers that forward call arguments.
djtodoro edited the summary of this revision. (Show Details)
djtodoro added a reviewer: bjope.

-Split up of introduction, production and handling of call site info

lebedev.ri added inline comments.
include/llvm/CodeGen/SelectionDAG.h
1633

The function is marked as const, yet this is a std::move().
Do you mean to only get the CallSiteInfo once?

NikolaPrica added inline comments.Apr 24 2019, 5:43 AM
include/llvm/CodeGen/SelectionDAG.h
1633

Too bad I got no warning on that. Thanks for pointing that!

Do you mean to only get the `CallSiteInfo` once?

Yes. For now it is used only in that way.

djtodoro updated this revision to Diff 196434.Apr 24 2019, 6:09 AM

-Remove const from getSDCallSiteInfo()

LGTM from my high-level point of view, but please make sure to address everybody else's feedback, too.

bjope added a comment.May 13 2019, 9:40 AM

FYI: I have no further comments.

aprantl added inline comments.May 13 2019, 11:57 AM
test/CodeGen/X86/call-site-info-output.ll
14

Is there *anything* we can check in the output here? Otherwise this will bitrot quickly.

djtodoro updated this revision to Diff 199791.May 16 2019, 5:04 AM

-Verify that call site info is the same after loading and running a pass over it.

aprantl accepted this revision.May 16 2019, 3:34 PM
This revision is now accepted and ready to land.May 16 2019, 3:34 PM
djtodoro updated this revision to Diff 201883.May 29 2019, 5:51 AM

-Follow the new order of fields inside the ArgRegPair struct (the struct is now better packed))

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 27 2019, 3:51 AM
Herald added a subscriber: jsji. · View Herald Transcript