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

[Daniil Troshkov]

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/be608754/attachment.html>