[PATCH] D36462: [CGP] Fix the rematerialization of gc.relocates

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added inline comments.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:951
   bool MadeChange = false;
+  // We must ensure the relocation of derived pointer is defined after
+  // relocation of base pointer. To achieve that we enumerate all GCRelocateInst
----------------
reames wrote:
> Doing this linear scan of the basic block once per gc-relocate seems needless expensive compile time wise.  I'd do one of the following:
> 1) Canonicalize the location of the base pointer before the derived relocations, run that code once per statepoint, then visit the gc-relocates only once it has run.
> 2) Reorganize the code to identify all remat candidates, then scan once for earliest required position, then materialize all including the moved base pointer.
> 
> Smaller comments:
> - Group the BB check with the scan code since they're all handling the same case.
> - Since we have to do the linear scan anyways, should we just handle the duplicate case at the same time?
It is not once per gc-relocate, it is once per RelocatedBase and all gc-relocate corresponding to RelocatedBase.

However I think I can do a bit smarter here. Specifically, I can check that the relocation I met before RelocatedBase is actually gc-relocate corresponding that base and move RelocatedBase right before this gc-relocate. So it should be faster and does not require utility map.

I will upload a new patch.


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:956
+  unsigned CurBaseIdx = 0;
+  for (auto &I : *RelocatedBase->getParent())
+    if (auto RI = dyn_cast<GCRelocateInst>(&I)) {
----------------
reames wrote:
> Rather than scanning all instructions in the block, you can start at the statepoint.
I cannot start with statepoint because it might be located in a different basic block then RelocateBase.


https://reviews.llvm.org/D36462