[llvm] r336664 - [DAGCombiner] visitREM - call visitSDIVLike/visitUDIVLike directly to avoid recursive combining.

[Simon Pilgrim via llvm-commits]

On 10/07/2018 19:47, Friedman, Eli wrote:

> On 7/10/2018 6:18 AM, Simon Pilgrim via llvm-commits wrote:
>> Author: rksimon
>> Date: Tue Jul 10 06:18:16 2018
>> New Revision: 336664
>>
>> URL: http://llvm.org/viewvc/llvm-project?rev=336664&view=rev
>> Log:
>> [DAGCombiner] visitREM - call visitSDIVLike/visitUDIVLike directly to 
>> avoid recursive combining.
>>
>> As suggested by @efriedma on D48975 use the 
>> visitSDIVLike/visitUDIVLike functions introduced at rL336656.
>>
>> Modified:
>>      llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>>
>> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp
>> URL: 
>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp?rev=336664&r1=336663&r2=336664&view=diff
>> ============================================================================== 
>>
>> --- llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp (original)
>> +++ llvm/trunk/lib/CodeGen/SelectionDAG/DAGCombiner.cpp Tue Jul 10 
>> 06:18:16 2018
>> @@ -3277,22 +3277,19 @@ SDValue DAGCombiner::visitREM(SDNode *N)
>>       // If X/C can be simplified by the division-by-constant logic, 
>> lower
>>     // X%C to the equivalent of X-X/C*C.
>> -  // To avoid mangling nodes, this simplification requires that the 
>> combine()
>> -  // call for the speculative DIV must not cause a DIVREM 
>> conversion.  We guard
>> -  // against this by skipping the simplification if 
>> isIntDivCheap().  When
>> -  // div is not cheap, combine will not return a DIVREM. Regardless,
>> -  // checking cheapness here makes sense since the simplification 
>> results in
>> -  // fatter code.
>> +  // Reuse the SDIVLike/UDIVLike combines - to avoid mangling nodes, 
>> the
>> +  // speculative DIV must not cause a DIVREM conversion.  We guard 
>> against this
>> +  // by skipping the simplification if isIntDivCheap().  When div is 
>> not cheap,
>> +  // combine will not return a DIVREM.  Regardless, checking 
>> cheapness here
>> +  // makes sense since the simplification results in fatter code.
>>     if (N1C && !N1C->isNullValue() && !TLI.isIntDivCheap(VT, Attr)) {
>> -    unsigned DivOpcode = isSigned ? ISD::SDIV : ISD::UDIV;
>> -    SDValue Div = DAG.getNode(DivOpcode, DL, VT, N0, N1);
>> -    AddToWorklist(Div.getNode());
>> -    SDValue OptimizedDiv = combine(Div.getNode());
>> -    if (OptimizedDiv.getNode() && OptimizedDiv.getNode() != 
>> Div.getNode() &&
>> -        OptimizedDiv.getOpcode() != ISD::UDIVREM &&
>> +    SDValue OptimizedDiv =
>> +        isSigned ? visitSDIVLike(N0, N1, N) : visitUDIVLike(N0, N1, N);
>> +    if (OptimizedDiv.getNode() && OptimizedDiv.getOpcode() != 
>> ISD::UDIVREM &&
>>           OptimizedDiv.getOpcode() != ISD::SDIVREM) {
>
> I think the check for SDIVREM/UDIVREM is redundant here? visitUDIVLike 
> can't return UDIVREM.
>
> -Eli
IIRC I replaced that initially with an assert for the DIVREMs but I was 
seeing hits - I'll look again. Simon.