[llvm] r192795 - DAGCombiner: Don't fold xor into not if getNOT would introduce an illegal constant.

[Daniel Sanders]

> -----Original Message-----
> From: Benjamin Kramer [mailto:benny.kra at gmail.com]
> Sent: 16 October 2013 17:24
> To: Daniel Sanders
> Cc: llvm-commits at cs.uiuc.edu
> Subject: Re: [llvm] r192795 - DAGCombiner: Don't fold xor into not if getNOT
> would introduce an illegal constant.
> 
> 
> On 16.10.2013, at 18:04, Daniel Sanders <Daniel.Sanders at imgtec.com>
> wrote:
> 
> > Hi,
> >
> > I think I'm working on a patch that's relevant to this. On MIPS32 with MSA
> we also have a legal v2i64 and an illegal i64 and I've had similar problems with
> the Legalizer and DAGCombiner introducing illegal types. There's a few other
> triggers such as legalizing v2i64 UNDEF.
> >
> > My patch causes SelectionDAG::getConstant() to produce an appropriate
> bitcasted build_vector when given a legal vector type whose elements are
> normally expanded by the type legalizer.
> >
> > I've just sent the patch series to my colleagues for our internal code
> review. I'll be posting the patches to llvm-commits afterwards but you can
> see the patches at https://dmz-
> portal.mips.com/bugz/show_bug.cgi?id=1237 if you want a sneak preview.
> Patch 5/5 is the most important one.
> 
> That definitely looks like it's going in the right direction, thanks. Some things
> worth considering:
> 
> * Do we really want to do this for all constants before legalize types? It looks
> like it adds an unnecessary layer of obfuscation at this level (optimizer has to
> look through bitcasts, etc). It also wouldn't surprise me if the DAGCombiner
> just folds it back into an illegal constant. Not a big problem, but creates
> unnecessary churn.

I agree it shouldn't be doing this before legalize types. I'll fix that.

> * What about <2 x i64> when only i16 is legal? I don't think we have such a
> target currently, but your approach would fail and split it into <4 x i16> which
> is wrong.

I might have misunderstood the API documentation which talks about "one step in the expansion to get to the smaller register". I thought that when i32 is also illegal, getTypeToTransformTo() would still return i32 because it's one step closer to a legal type. I expected that further (currently unwritten) expansions to be required to get to <8 x i16>. If it would return i16 then I can rewrite this code into a loop easily enough.

> - Ben
> >
> >> -----Original Message-----
> >> From: llvm-commits-bounces at cs.uiuc.edu [mailto:llvm-commits-
> >> bounces at cs.uiuc.edu] On Behalf Of Benjamin Kramer
> >> Sent: 16 October 2013 15:16
> >> To: llvm-commits at cs.uiuc.edu
> >> Subject: [llvm] r192795 - DAGCombiner: Don't fold xor into not if
> >> getNOT would introduce an illegal constant.
> >>
> >> Author: d0k
> >> Date: Wed Oct 16 09:16:19 2013
> >> New Revision: 192795
> >>
> >> URL: http://llvm.org/viewvc/llvm-project?rev=192795&view=rev
> >> Log:
> >> DAGCombiner: Don't fold xor into not if getNOT would introduce an
> >> illegal constant.
> >>
> >> This happens e.g. with <2 x i64> -1 on x86_32. It cannot be generated
> >> directly because i64 is illegal. It would be nice if getNOT would
> >> handle this transparently, but I don't see a way to generate a legal
> >> constant there right now. Fixes PR17487.
> >>
> >> Modified:
> >>    llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> >>    llvm/trunk/test/CodeGen/X86/xor.ll
> >>
> >> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
> >> URL: http://llvm.org/viewvc/llvm-
> >>
> project/llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp?rev=192
> >> 795&r1=192794&r2=192795&view=diff
> >>
> ==========================================================
> >> ====================
> >> --- llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (original)
> >> +++ llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp Wed Oct
> 16
> >> +++ 09:16:19 2013
> >> @@ -3583,7 +3583,7 @@ SDValue DAGCombiner::visitXOR(SDNode *N)
> >>   }
> >>   // fold (xor (and x, y), y) -> (and (not x), y)
> >>   if (N0.getOpcode() == ISD::AND && N0.getNode()->hasOneUse() &&
> >> -      N0->getOperand(1) == N1) {
> >> +      N0->getOperand(1) == N1 && isTypeLegal(VT.getScalarType())) {
> >>     SDValue X = N0->getOperand(0);
> >>     SDValue NotX = DAG.getNOT(SDLoc(X), X, VT);
> >>     AddToWorkList(NotX.getNode());
> >>
> >> Modified: llvm/trunk/test/CodeGen/X86/xor.ll
> >> URL: http://llvm.org/viewvc/llvm-
> >>
> project/llvm/trunk/test/CodeGen/X86/xor.ll?rev=192795&r1=192794&r2=19
> >> 2795&view=diff
> >>
> ==========================================================
> >> ====================
> >> --- llvm/trunk/test/CodeGen/X86/xor.ll (original)
> >> +++ llvm/trunk/test/CodeGen/X86/xor.ll Wed Oct 16 09:16:19 2013
> >> @@ -165,3 +165,17 @@ define <4 x i32> @test10(<4 x i32> %a) n  ; X32-
> >> LABEL: test10:
> >> ; X32:    andnps
> >> }
> >> +
> >> +define i32 @PR17487(i1 %tobool) {
> >> +  %tmp = insertelement <2 x i1> undef, i1 %tobool, i32 1
> >> +  %tmp1 = zext <2 x i1> %tmp to <2 x i64>
> >> +  %tmp2 = xor <2 x i64> %tmp1, <i64 1, i64 1>
> >> +  %tmp3 = extractelement <2 x i64> %tmp2, i32 1
> >> +  %add = add nsw i64 0, %tmp3
> >> +  %cmp6 = icmp ne i64 %add, 1
> >> +  %conv7 = zext i1 %cmp6 to i32
> >> +  ret i32 %conv7
> >> +
> >> +; X64-LABEL: PR17487:
> >> +; X64: andn
> >> +}
> >>
> >>
> >> _______________________________________________
> >> llvm-commits mailing list
> >> llvm-commits at cs.uiuc.edu
> >> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
> >