[PATCH] D40497: [PowerPC] Partially enable the ISEL expansion pass.

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

I would prefer to have an MIR test case for both cases so we can test that we do the right thing when we come across these redundant/foldable ISEL's. Furthermore, it would be good to have an IR test case for both cases with comments as to what transformation has lead to the redundant case - so 2 IR and 2 MIR test cases.



================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:204
+
+      // Special case 1, all registers used by ISEL are the same one.
+      if (useSameRegister(Dest, TrueValue) &&
----------------
I would add a comment here that the non-redundant `isel 0, 0, 0, N` would not satisfy these conditions as it would be `ISEL %R0, %ZERO, %R0, %CRN`.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:209
+        NumRemoved++;
+        (*I)->eraseFromParent();
+        I++;
----------------
This will erase it from the MBB but not from this data structure that we've got all the ISEL instructions in. Is that safe? Are we guaranteed not to iterate over this thing again and blow up?
I'd personally prefer not to have this dangling pointer to a deleted instruction.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:217
+        NumFolded++;
+        BuildMI(*MBB, (*I), dl, TII->get(isISEL8(**I) ? PPC::ADDI8 : PPC::ADDI))
+            .add(Dest)
----------------
Why do we not use the standard opcode for copying a register since other passes know about that opcode? Namely `MR` or rather the base opcode `OR`.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:221
+            .add(MachineOperand::CreateImm(0));
+        (*I)->eraseFromParent();
+        I++;
----------------
Same comment regarding the dangling pointer to the ISEL instruction.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:229
+          SubISELList.push_back(*I++);
+          // Collect the ISELs that can be merged together.
+          while (I != E && canMerge(SubISELList.back(), *I)) {
----------------
Add a comment here that this will eat up ISEL instructions without considering whether they may be redundant or foldable to a register copy. So we still keep the `handleSpecialCases()` function in case we've done this.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:279
     // Special case 2, the two input registers used by ISEL are the same.
     // Note 1: We favor merging ISEL expansions over folding a single one. If
     // the passed list has multiple merge-able ISEL's, we won't fold any.
----------------
jtony wrote:
> syzaara wrote:
> > jtony wrote:
> > > syzaara wrote:
> > > > It seems that Note 1 is no longer satisfied since you have already folded the ISELs in expandAndMergeISELs.
> > > Same here.
> > If we have the case of:
> > foldable isel
> > foldable isel
> > regular isel
> > 
> > where both the foldable isels can be merged, previously with expansion enabled, we would merge them. Now, with the expansion enabled, they will be changed to move registers before. So we won't be getting the same behavior with the expansion enabled, making this comment invalid.
> That's right Zaara. We could guard the first two special cases handling inside expandAndMergeISELs() with condition `if (!ExpandISELEnabled)` if we really want to keep the the behavior in the original implementation. Or If we are OK for the change, we just need to update the comments here. @nemanjai  Do you have any comments? 
I would not try to keep the old semantics. The comment is actually semantically valid. It is true that "if the passed list has multiple mergeable ISEL's, we won't fold any". However, due to the folding that has happened previously, it is less likely that the list will actually contain any foldable ISEL's.

Or am I misunderstanding what this discussion is about?


================
Comment at: test/CodeGen/PowerPC/expand-contiguous-isel.ll:123
 ; CHECK-LABEL: @_Z3fn1N4llvm9StringRefE
-; CHECK-GEN-ISEL-TRUE: isel [[SAME:r[0-9]+]], [[SAME]], [[SAME]]
+; Unecessary ISEL (all the registers are the same) is always removed
+; CHECK-GEN-ISEL-TRUE-NOT: isel [[SAME:r[0-9]+]], [[SAME]], [[SAME]]
----------------
Please track down what transformation is producing the redundant ISEL and add that note here.


https://reviews.llvm.org/D40497