[PATCH] Passing down BinaryOperator flags to BinarySDNode + X86 optimization

Marcello Maggioni hayarms at gmail.com
Wed Jun 4 01:28:03 PDT 2014


Whoops, the autocorrection got me :P

What I meant is: if somebody with commit access agrees with this patch can
commit it please? :)

Thanks,
Marcello


2014-06-04 0:46 GMT-07:00 Marcello Maggioni <hayarms at gmail.com>:

> Thank you very much for your comments Daniil !
>
> If anybody that commit access agrees also with these patches can somebody
> commit them please? :)
>
> Cheers,
> Marcello
>
>
> 2014-06-04 0:35 GMT-07:00 Daniil Troshkov <troshkovdanil at gmail.com>:
>
> ok
>>
>>
>> On Wed, Jun 4, 2014 at 10:30 AM, Marcello Maggioni <hayarms at gmail.com>
>> wrote:
>>
>>> Here is an updated patch with the cases grouped in a macro.
>>>
>>> I made the syntax of the macro such that it looks like when put in a
>>> switch it looks like  a regular switch case.
>>>
>>> I couldn't find a better way to address this sadly :/
>>>
>>> The optimization patch remained the same, but I updated the version
>>> number.
>>>
>>> Marcello
>>>
>>>
>>> 2014-06-03 8:04 GMT-07:00 Marcello Maggioni <hayarms at gmail.com>:
>>>
>>> Thanks. The macro could be an idea for that, yeah. I'll try to see if
>>>> there is an alternative solution for this and in case I cannot find one I
>>>> will implement your suggestion.
>>>>
>>>> About the differences:
>>>>
>>>> A and B should be the same (probably the order of the labels is
>>>> different, but they are the same) and these represent the nodes that have
>>>> binop flags attached to them.
>>>>
>>>> In the optimisation code the switch cases are different because I was
>>>> targeting a specific case identified by those instructions. (A subset of
>>>> the nodes that have flags)
>>>>
>>>> Marcello
>>>>
>>>> Il martedì 3 giugno 2014, Daniil Troshkov <troshkovdanil at gmail.com> ha
>>>> scritto:
>>>>
>>>> 1) Ok
>>>>> 2) Ok
>>>>> 3) I mean:
>>>>>
>>>>> A)
>>>>> +  /// isBinOpWithFlags - Returns true if the opcode is a binary
>>>>> operation
>>>>> +  /// with flags.
>>>>> +  static bool isBinOpWithFlags(unsigned Opcode) {
>>>>> +    switch (Opcode) {
>>>>> +    case ISD::ADD:
>>>>> +    case ISD::MUL:
>>>>> +    case ISD::SUB:
>>>>> +    case ISD::SDIV:
>>>>> +    case ISD::UDIV:
>>>>> +    case ISD::SRL:
>>>>> +    case ISD::SRA:
>>>>> +    case ISD::SHL: return true;
>>>>> +    default: return false;
>>>>> +    }
>>>>> +  }
>>>>>
>>>>> B)
>>>>> @@ -473,6 +487,19 @@ static void AddNodeIDCustom(FoldingSetNodeID &ID,
>>>>> const SDNode *N) {
>>>>>      ID.AddInteger(ST->getPointerInfo().getAddrSpace());
>>>>>      break;
>>>>>    }
>>>>> +  case ISD::SDIV:
>>>>> +  case ISD::UDIV:
>>>>> +  case ISD::SRA:
>>>>> +  case ISD::SRL:
>>>>> +  case ISD::MUL:
>>>>> +  case ISD::ADD:
>>>>> +  case ISD::SUB:
>>>>> +  case ISD::SHL: {
>>>>> +    const BinarySDNode *BinNode = static_cast<const BinarySDNode*>(N);
>>>>> +    AddBinaryNodeIDCustom(ID, N->getOpcode(),
>>>>> BinNode->hasNoUnsignedWrap(),
>>>>> +                          BinNode->hasNoSignedWrap(),
>>>>> BinNode->isExact());
>>>>> +    break;
>>>>> +  }
>>>>>    case ISD::ATOMIC_CMP_SWAP:
>>>>>    case ISD::ATOMIC_SWAP:
>>>>>    case ISD::ATOMIC_LOAD_ADD:
>>>>>
>>>>> and
>>>>>
>>>>> C)
>>>>> +    switch (Op->getOpcode()) {
>>>>> +      case ISD::ADD:
>>>>> +      case ISD::SUB:
>>>>> +      case ISD::MUL:
>>>>> +      case ISD::SHL: {
>>>>> +        const BinarySDNode *BinNode =
>>>>> +          static_cast<const BinarySDNode*>(Op.getNode());
>>>>> +        if (BinNode->hasNoSignedWrap())
>>>>> +          break;
>>>>> +      }
>>>>> +      default:
>>>>> +        NeedOF = true;
>>>>> +        break;
>>>>> +    }
>>>>>
>>>>> I don't like this code duplication.
>>>>> Maybe using #define CASE_BINOP case ISD::SDIV:\
>>>>>  case ISD::UDIV: \
>>>>> ....case ISD::SHL
>>>>> to avoid this code duplication?
>>>>> But I'm not sure this is good idea...
>>>>> One more question: why we have difference between A) B) and C)?
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> On Tue, Jun 3, 2014 at 4:01 AM, Marcello Maggioni <hayarms at gmail.com>
>>>>> wrote:
>>>>>
>>>>> Hi, thanks Daniil for your review!
>>>>>
>>>>> 1) Yeah, I also kind of like what you proposed more ... I used the
>>>>> approach that is used currently in BinaryOperator for that. I'm not sure if
>>>>> it is better to change both to this format or having them to diverge
>>>>> (keeping BinaryOperator like it was and changing this one to the syntax you
>>>>> proposed).
>>>>>
>>>>> 2) Thanks for spotting those stylistic inconsistencies! I'm working on
>>>>> a new setup and I think I still have to tune some of my editor default
>>>>> settings ...
>>>>>
>>>>> 3) What you mean exactly with this one?
>>>>>
>>>>> PS = Here are some updated patches with concerns number 1 and 2
>>>>> addressed.
>>>>>
>>>>> Thanks,
>>>>> Marcello
>>>>>
>>>>>
>>>>> 2014-06-02 17:00 GMT-07:00 Marcello Maggioni <hayarms at gmail.com>:
>>>>>
>>>>> Hi, thanks Daniil for your review!
>>>>>
>>>>> 1) Yeah, I also kind of like what you proposed more ... I used the
>>>>> approach that is used currently in BinaryOperator for that. I'm not sure if
>>>>> it is better to change both to this format or having them to diverge
>>>>> (keeping BinaryOperator like it was and changing this one to the syntax you
>>>>> proposed).
>>>>>
>>>>> 2) Thanks for spotting those stylistic inconsistencies! I'm working on
>>>>> a new setup and I think I still have to tune some of my editor default
>>>>> settings ...
>>>>>
>>>>> 3) What you mean exactly with this one?
>>>>>
>>>>> PS = Here are some updated patches with concerns number 1 and 2
>>>>> addressed.
>>>>>
>>>>> Thanks,
>>>>> Marcello
>>>>>
>>>>>
>>>>> 2014-06-02 2:36 GMT-07:00 Данил Трошков <troshkovdanil at mail.ru>:
>>>>>
>>>>>  1)
>>>>>
>>>>>      (SubclassData & ~NUW) | (b * NUW);
>>>>> Maybe better to write somehow
>>>>>
>>>>> (SubclassData & ~NUW) | (b ? NUW : 0);
>>>>> IMHO it is more readable
>>>>>
>>>>> 2) const BinarySDNode *BinNode = static_cast<const BinarySDNode*>(N);
>>>>> formatting: \t...
>>>>> +                BinNode->hasNoUnsignedWrap(),
>>>>> +         BinNode->hasNoSignedWrap(),
>>>>> +         BinNode->isExact());
>>>>> the same...
>>>>> +                    bool nsw, bool exact) {
>>>>> +          bool nsw, bool exact) {
>>>>> +        Op1, Op2, HasNUW, HasNSW, IsExact);
>>>>> + if (BinNode->hasNoSignedWrap())
>>>>> +   break;
>>>>>
>>>>> 3) Maybe better to remove bundle of cases on low level?
>>>>> In order to reduce duplication of code...
>>>>>
>>>>>
>>>>>
>>>>>  Sat, 31 May 2014 19:30:24 -0700 от Marcello Maggioni <
>>>>> hayarms at gmail.com>:
>>>>>
>>>>>   Here an updated patch.
>>>>>
>>>>> The changes are:
>>>>>
>>>>> - I stripped away the new getBinaryNode function and integrated the
>>>>> functionality in the getNode() function with two operands.
>>>>> - I removed the refactoring for the binary folding logic. Removing
>>>>> getBinaryNode() makes putting that logic in an external function not
>>>>> needed. (this also fixes the clang-format problem Owen pointed out).
>>>>> - Cleaned up some stuff and fixe
>>>>>
>>>>>
>>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140604/62e72141/attachment.html>


More information about the llvm-commits mailing list