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

Marcello Maggioni hayarms at gmail.com
Tue Jun 3 23:30:06 PDT 2014


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/20140603/b7afc472/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: flag_nodes_v3.patch
Type: application/octet-stream
Size: 12702 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140603/b7afc472/attachment.obj>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: x86_opt_wrap_v3.patch
Type: application/octet-stream
Size: 2774 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140603/b7afc472/attachment-0001.obj>


More information about the llvm-commits mailing list