[PATCH] D94264: [GlobalISel] Add MachineInstNumbering to CSEInfo and propagate CSE throughout AArch64 pipeline.

[Jessica Paquette via Phabricator via llvm-commits]

paquette added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/CSEInfo.h:109
+  void flushAndRenumber();
+  /// Remove an instruction from SlotIndexes.
+  void removeInstruction(MachineInstr &MI);
----------------
Maybe good to note that this also removes `MI` from the queue if it is present?


================
Comment at: llvm/include/llvm/CodeGen/GlobalISel/MachineIRBuilder.h:372
   MachineInstrBuilder buildInstr(unsigned Opcode) {
-    return insertInstr(buildInstrNoInsert(Opcode));
+    auto MIB = insertInstr(buildInstrNoInsert(Opcode));
+    return MIB;
----------------
Why is this necessary?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CSEInfo.cpp:215
+  // Remove the instruction from the queue if it exists. Due to a bug that
+  // inserts instructions twice due to duplicate observers, we check the entire
+  // queue instead of quitting on the first hit.
----------------
Does this bug have a FIXME or something somewhere?


================
Comment at: llvm/lib/CodeGen/GlobalISel/CSEInfo.cpp:356
+  do {
+    --II;
+    MachineInstr *CurrInst = &*II;
----------------
I think you can remove the check for `CurrInst->isDebugInstr()` if you use `skipDebugInstructionsBackward`.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CSEInfo.cpp:380
+  do {
+    ++II;
+    if (II == ParentBB->end())
----------------
This function and `findEarlierNonQueuedInst` are the same aside from the direction they iterate in.

Is it possible to refactor them somehow so they can share more code?

`findEarlierNonQueuedInst` looks like it's the same loop if you use a `reverse_iterator`.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CSEInfo.cpp:380
+  do {
+    ++II;
+    if (II == ParentBB->end())
----------------
paquette wrote:
> This function and `findEarlierNonQueuedInst` are the same aside from the direction they iterate in.
> 
> Is it possible to refactor them somehow so they can share more code?
> 
> `findEarlierNonQueuedInst` looks like it's the same loop if you use a `reverse_iterator`.
I think you can remove the check for `CurrInst->isDebugInstr()` if you use `skipDebugInstructionsForward`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94264