[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:
> > > > 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.
> Sounds work. However, I still have some concern that, sext really is cheaper  than zext in PowerPC ? The reason why we want to do signed extend is that, if will break some pattern for select combine rule, not because of it is cheaper...
> 
> Anyway, I will try it first.
> Inside SelectionDAG::getNode, call hook TLI.isSExtCheaperThanZExt to decide how to promote the any extend constant.
> 
> 
> Sounds work. However, I still have some concern that, sext really is cheaper than zext in PowerPC ? 

I think that it depends on the context; but any_extend is supposed to allow us to do something smart (and target influenced), and so let's see if we can make it a little smarter and get a broader benefit.


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

https://reviews.llvm.org/D63590