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

[Simon Atanasyan via Phabricator via llvm-commits]

atanasyan 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()) {
----------------
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?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61787