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

[Benjamin Kramer]

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.

* 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.

- 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
>