[PATCH] D95703: [GlobalISel] Propagate extends through G_PHIs into the incoming value blocks.

[Amara Emerson via Phabricator via llvm-commits]

aemerson added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:3589
+
+  // If the target is likely to fold this extend away, don't propagate.
+  if (Builder.getTII().isExtendLikelyToBeFolded(*ExtMI, MRI))
----------------
arsenm wrote:
> Folded into what?
Could be anything. The idea is that extensions which are "free" because they'd be folded into another operation, are not worth propagating.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:3597
+  SmallPtrSet <MachineInstr*, 4> InSrcs;
+  for (unsigned Idx = 1; Idx < MI.getNumOperands(); Idx += 2)
+    InSrcs.insert(getDefIgnoringCopies(MI.getOperand(Idx).getReg(), MRI));
----------------
paquette wrote:
> This can have at most 3 elements before returning false, so I guess you might as well cut the loop short here.
Unless I'm misunderstanding, this would prevent us from knowing how many unique incoming values there are, if we stop early. G_PHIs can have any number of operands.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:3605
+
+  for (auto *DefMI : InSrcs) {
+    switch (DefMI->getOpcode()) {
----------------
paquette wrote:
> This can probably just go in the loop above, which should allow you to bail out early as well.
Sure.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp:3632
+  SetVector<MachineInstr *> SrcMIs;
+  DenseMap<MachineInstr *, MachineInstr *> OldToNewSrcMap;
+  for (unsigned SrcIdx = 1; SrcIdx < MI.getNumOperands(); SrcIdx += 2) {
----------------
paquette wrote:
> Why not `SmallDenseMap`?
Yeah, will change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95703