[PATCH] D61787: [GlobalISel Legalizer] Improve artifact combiner

[Petar Avramovic via Phabricator via llvm-commits]

Petar.Avramovic marked an inline comment as done.
Petar.Avramovic added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/Legalizer.cpp:336
+    // like instructions. Do this after we have processed everything else.
+    if (InstList.empty() && ArtifactList.empty() &&
+        TryLaterArtifactList.empty()) {
----------------
atanasyan wrote:
> Petar.Avramovic wrote:
> > atanasyan wrote:
> > > It looks like if move this block out of the `while` loop we can drop the `if` statement and write it as:
> > > ```
> > > SmallVector<MachineInstr *, 8> Flip;
> > > while (!AuxiliaryArtifactList.empty())
> > >   Flip.push_back(AuxiliaryArtifactList.pop_back_val());
> > > while (!Flip.empty())
> > >   InstList.insert(Flip.pop_back_val());
> > > ```
> > We have to move AuxiliaryArtifactList to InstList inside while loop because otherwise whatever is left in AuxiliaryArtifactList will remain unprocessed and will affect regression tests.
> > 
> > Now that I look at this, it might be better to move Auxiliary Artifacts to InstList one by one 
> > ```
> >     if (InstList.empty() && ArtifactList.empty() &&
> >         TryLaterArtifactList.empty() && !AuxiliaryArtifactList.empty())
> >       InstList.insert(AuxiliaryArtifactList.pop_back_val());
> > ```
> > instead of all together, this could allow us to combine away Auxiliary Artifact with help of an Artifact that appears after some other Auxiliary Artifacts was legalized. I don't have a test for this sequence of events.
> > 
> > Or maybe even to Artifact list if there appears combine where  Auxiliary Artifact could be an Artifact ( e.g. G_TRUNC + G_MERGE_VALUES) but then some opcodes (G_TRUNC) will have dual meaning and will be hard to figure out what combine have precedence over another.
> > 
> > Suggestions?
> > 
> > Currently, for MIPS, I would expect for AuxiliaryArtifactList to be empty when all other lists are empty.
> > This patch is required for MIPS to allow legalization of chained artifacts that appear when we narrow Scalar G_{S|Z}EXT_INREG (once D61289 lands) or narrow scalar G_{Z|S|ANY}EXT.
> > We have to move AuxiliaryArtifactList to InstList inside while loop because otherwise whatever is left in AuxiliaryArtifactList will remain unprocessed and will affect regression tests.
> 
> Agreed. I missed that in the `if` condition has `TryLaterArtifactList.empty()` while in the loop cindition has `!TryLaterArtifactList.empty()`.
> 
> > it might be better to move Auxiliary Artifacts to InstList one by one
> 
> If you put this statement in the end of the loop the last element of the `AuxiliaryArtifactList` will be moved to the `InstList`. After that we exit from the loop because both `InstList.empty()` and `TryLaterArtifactList.empty()` are true and the loop condition `!InstList.empty() || !TryLaterArtifactList.empty()` is false. If `AuxiliaryArtifactList` contains more then one element they remains in the list. Did I miss something?
Once we move top element (Art) from AuxiliaryArtifactList to InstList, InstList is no longer empty and we enter loop again with intention to legalize Art. 

Remaining (if any) auxiliary artifacts in AuxiliaryArtifactList stay there until we are done with legalizing Art and everything he produced during legalization.
It may be possible that this Art could produce e.g. G_UNMERGE that could combine away some G_MERGE that is still in AuxiliaryArtifactList. Again this is just in theory, I don't have test for this case.
For this to happen, there can be no artifact that attempted to combine with Art. That is Art is only used in legal instructions. This look a little strange considering how we deal with artifacts at the moment, but might be necessary in the future as more artifact combines are added.



Repository:
  rL LLVM

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

https://reviews.llvm.org/D61787