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

[Tony Jiang via Phabricator via llvm-commits]

jtony marked 2 inline comments as done.
jtony added inline comments.


================
Comment at: lib/Target/PowerPC/PPCExpandISEL.cpp:268
 
     // Special case 1, all registers used by ISEL are the same one.
     if (!IsADDIInstRequired && !IsORIInstRequired) {
----------------
syzaara wrote:
> Won't these special cases already be handled by expandAndMergeISELs?
That's a very good question.  I considered about that. The majority of these two special cases should have been handled upstream. But because the way we traverse all the ISELs (non  backtracking) in the case we have consecutive ISELs which have the same condition（could be merged together) and if the first one is not these two special cases,  we will put all the mergeable ISEL in  the list and handle them here (expand all of them in the same if-then-else branches)  . Therefore, it is still possible we could come here. e.g: 
``` %r3 = ISEL %r5, %r4, %cr0gt
     %r3 = ISEL %r4, %r4, %cr0gt   <---- this is special cases 2
    %r3 = ISEL %r3, %r3, %cr0gt   <---- this is special cases 1
    %r3 = ISEL %r7, %r8, %cr0gt```

But if we disabled ISEL-expansion, we are guaranteed to remove all the special cases upstream in the expandAndMergeISELs (which is our mainly cases, since the expansion is disabled)



================
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.
----------------
syzaara wrote:
> It seems that Note 1 is no longer satisfied since you have already folded the ISELs in expandAndMergeISELs.
Same here.


https://reviews.llvm.org/D40497