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

[Petar Avramovic via Phabricator via llvm-commits]

Petar.Avramovic marked 2 inline comments as done.
Petar.Avramovic added inline comments.


================
Comment at: lib/CodeGen/GlobalISel/Legalizer.cpp:307
+            MachineInstr *UseInst = MRI.getVRegDef(UseOp.getReg());
+            // Use is an unresolved artifact or is in InstList.
+            // InstList now contains instructions that used to be artifacts or
----------------
atanasyan wrote:
> s/Use/UseInst/ in the comment?
Yes, comment does not do best job explaining what do here. We can simplify this if statement and change comment like this:
```
            // UseInst is an unresolved artifact or instruction.
            // Unresolved artifacts are in ArtifactList or TryLaterArtifactList.
            // InstList now contains only unresolved instructions i.e.
            // instructions that used to be artifacts or instructions created by
            // artifact combiner.
            if (ArtifactList.contains(UseInst) ||
                TryLaterArtifactList.contains(UseInst) ||
                InstList.contains(UseInst)) {
              HasUnresolvedUses = true;
              break;
            }
```



================
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:
> 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.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61787