[PATCH] D60715: [ISEL] Collect argument's forwarding regs when lowering calls

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

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).



================
Comment at: lib/CodeGen/TargetInstrInfo.cpp:138
 
-  // Remove all the dead instructions from the end of MBB.
-  MBB->erase(Tail, MBB->end());
+  // Remove all the dead instructions from the end of MBB and update call site
+  // info.
----------------
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).


================
Comment at: test/CodeGen/X86/call-site-info-output.ll:7
+; CHECK: callSites:
+; CHECK-NEXT: bb: 0, offset: 14, fwdArgRegs:
+; CHECK-NEXT:   arg: 0, reg: '$edi'
----------------
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?


================
Comment at: test/CodeGen/X86/call-site-info-output.ll:11
+; CHECK-NEXT:   arg: 2, reg: '$edx'
+; RUN: llc %t.mir -start-after=expand-isel-pseudos
+
----------------
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.



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60715/new/

https://reviews.llvm.org/D60715