[PATCH] D63590: [PowerPC] Sign extend the select instr operands if it is any_extend

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:12325
       else
-        Ops[C+i] = DAG.getAnyExtOrTrunc(Ops[C+i], dl, N->getValueType(0));
+        llvm_unreachable("Unexpected opcode!");
     }
----------------
steven.zhang wrote:
> hfinkel wrote:
> > steven.zhang wrote:
> > > hfinkel wrote:
> > > > This seems like we're working around a problem elsewhere. Why does forcing any_extend to become a sign_extend produce better code than just producing an any_extend?
> > > For any_extend, it is handled as zero extend if it is constant. For example, 
> > > ```
> > > t1: i8 = select t0, Constant:i8<-1>, Constant:i8<0>
> > > t2: i64 = any_extend t1
> > > ```
> > > It  will be combined as follows, for now:
> > > ```
> > > t3: i64 = select t0, Constant:i64<255>, Constant:i64<0>
> > > ```
> > > And the literal "255" is just a normal constant, therefore, the combine to inreg rule won't apply. This is what we expected:
> > > ```
> > > t3: i64 = select t0, Constant:i64<-1>, Constant:i64<0>
> > > ```
> > > So that, the combine to inreg rule would apply and it is combined as:
> > > t4: i64 = sign_extend_inreg t3
> > > 
> > > Doing zero extend will hurt some opportunities, while sign extend indeed helps. And we haven't seen the downside yet.
> > > Doing zero extend will hurt some opportunities, while sign extend indeed helps. And we haven't seen the downside yet.
> > 
> > What happens if we always prefer to lower any_extend to sign_extend instead of just doing it in this special case? Given that, on PPC, immediates are generally sign extended, etc., maybe that should be the default?
> > 
> Yes, I tried. For powerpc, I didn't see any problems by doing this, but also no benefit from the sanity test and benchmark 2017, but for X86, I indeed see many bad changes. I guess they have some assumption on this. So, I decide to just do it for aext + select. 
Is this the code that's going it:

    case ISD::ANY_EXTEND:
    case ISD::ZERO_EXTEND:
      return getConstant(Val.zextOrTrunc(VT.getSizeInBits()), DL, VT,
                         C->isTargetOpcode(), C->isOpaque());

(in SelectionDAG::getNode)? 

Also, maybe we should override TLI.isSExtCheaperThanZExt and propose tying this behavior to the result of that callback? It seems like we should be able to change this on PPC without affecting x86.


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

https://reviews.llvm.org/D63590