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

[Amara Emerson via Phabricator via llvm-commits]

aemerson added a comment.

In D94264#2580058 <https://reviews.llvm.org/D94264#2580058>, @arsenm wrote:

> In D94264#2579990 <https://reviews.llvm.org/D94264#2579990>, @aemerson wrote:
>
>> In D94264#2579954 <https://reviews.llvm.org/D94264#2579954>, @arsenm wrote:
>>
>>> I think legalizing/combining will do way, way more insertions than would happen in regalloc. Is this losing time to renumberings?
>>
>> On average, this does cost more in renumbering/flushing than it saves. It only provides a net benefit on the pathological cases in legalization & localization.
>>
>> In D94264#2579954 <https://reviews.llvm.org/D94264#2579954>, @arsenm wrote:
>>
>>> Would it make sense to increase the increment size?
>>
>> What do you mean by increment size?
>
> The instruction index values initially increment by 16 so you only renumber for every 15 or so insertions between instructions:
>
>   enum {
>     /// The default distance between instructions as returned by distance().
>     /// This may vary as instructions are inserted and removed.
>     InstrDist = 4 * Slot_Count
>   };

Ok, I'll try that and see what the effect is.



================
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;
----------------
paquette wrote:
> Why is this necessary?
Must be vestigial.


================
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.
----------------
paquette wrote:
> Does this bug have a FIXME or something somewhere?
I think Matt fixed this by removing the observer from the MIRBuilder a while ago, which landed after I wrote this.


================
Comment at: llvm/lib/CodeGen/GlobalISel/CSEInfo.cpp:380
+  do {
+    ++II;
+    if (II == ParentBB->end())
----------------
paquette wrote:
> 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`.
I'll see what I can do here. I did try to factor things out originally but ran into a a few issues.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94264