[PATCH] D119916: Add a machine function pass to convert binop(phi(constants), v) to phi(binop)

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added a comment.

Partial review



================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:1583
+  /// This function must be synchronizated with FoldImmediate.
+  virtual bool canFoldImmediate(MachineInstr &UseMI, Register UseReg,
+                                int64_t ImmVal,
----------------
Could you add a comment explaining the relationship between `UseReg` and the other arguments?

I'm guessing `UseReg` is what defines `ImmVal`.
In other words, putting all the arguments together, we're looking at a pattern like this:
```
UseReg = loadImm ImmVal
... = UseMI(..., UseReg)
```

While at it, would be worth adding a `\pre UseReg is used in UseMI`.


================
Comment at: llvm/include/llvm/CodeGen/TargetInstrInfo.h:1583
+  /// This function must be synchronizated with FoldImmediate.
+  virtual bool canFoldImmediate(MachineInstr &UseMI, Register UseReg,
+                                int64_t ImmVal,
----------------
qcolombet wrote:
> Could you add a comment explaining the relationship between `UseReg` and the other arguments?
> 
> I'm guessing `UseReg` is what defines `ImmVal`.
> In other words, putting all the arguments together, we're looking at a pattern like this:
> ```
> UseReg = loadImm ImmVal
> ... = UseMI(..., UseReg)
> ```
> 
> While at it, would be worth adding a `\pre UseReg is used in UseMI`.
Side remarks.
I wonder if we should acknowledge here that `TTI` also has `isLegalXXXImmediate` and how this is different.

The only reason I mention it is because when I first look at this patch the new API looked familiar and I was wondering if we were re-implementing something that already exists.


================
Comment at: llvm/lib/CodeGen/DupConstPhiUsers.cpp:10
+// \file This pass finds out PHIs whose operands are constants, duplicates its
+// user to predecessors and folds in the constants.
+//
----------------
You mentioned in the description of the PR that this pass does both code size and performance improvements.
May be worth saying that here again.

I haven't read the rest of the patch yet, but it would be good to describe the cost model as well somewhere.
E.g., what would happen with something like:
```
a = ldimm cst1
if ()
  b = ldimm cst2
c = phi(a, b)
= add ..., c
```

What prevents us for having 2 adds on the path `a -> b -> c`, which I believe would be a perf regression.


================
Comment at: llvm/lib/CodeGen/DupConstPhiUsers.cpp:126
+  MachineInstr *FindSinglePhyRegUser(Register PhyReg, MachineInstr *CopyMI);
+  bool FindCandidateInfo(CandidateInfo &Candidate);
+  bool isMISafeToMove(MachineInstr *MI, CandidateInfo &Candidate);
----------------
Function names should start with lower case.


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

https://reviews.llvm.org/D119916