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

[Nikola Prica via Phabricator via llvm-commits]

NikolaPrica added a comment.

@bjope @aprantl Thank you for your comments!

In D60715#1469515 <https://reviews.llvm.org/D60715#1469515>, @aprantl wrote:

> 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?



================
Comment at: include/llvm/CodeGen/MIRYamlMapping.h:340
 
+struct CallSiteInfo {
+  struct ArgRegPair {
----------------
aprantl wrote:
> This could use some Doxygen comments :-)
Oh sorry about that. Will add those!


================
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.
----------------
bjope wrote:
> 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).
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); `?


================
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
+
----------------
bjope wrote:
> 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.
> 
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.


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

https://reviews.llvm.org/D60715