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

[Tony Jiang via Phabricator via llvm-commits]

jtony marked 9 inline comments as done.
jtony added a comment.

In https://reviews.llvm.org/D40497#938210, @echristo wrote:

> How are we generating these isels if they're basically nops?


They are introduced at simple register coalensing stage. For details please see the comments I added in the test cases.



================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:209
+        NumRemoved++;
+        (*I)->eraseFromParent();
+        I++;
----------------
nemanjai wrote:
> 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.
Yes, it is safe. Here the data structure is traversed once only.


================
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.
----------------
nemanjai wrote:
> 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?
You understanding is right. As long as we don't want to keep the old semantics, we are OK here.


https://reviews.llvm.org/D40497