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

[Andrea Di Biagio]

On Thu, Jun 5, 2014 at 8:50 PM, Marcello Maggioni <hayarms at gmail.com> wrote:
> Hi Andrea,
>
> answers are inlined!
>
> <snip>
>> About patch "flag_nodes_v4.patch"
>>
>> In include/llvm/CodeGen/SelectionDAG.h:
>>
>> +  /// isBinOpWithFlags - Returns true if the opcode is a binary operation
>> +  /// with flags.
>> +  static bool isBinOpWithFlags(unsigned Opcode) {
>> +    switch (Opcode) {
>> +    case BINOP_NODES_W_FLAGS: return true;
>> +    default: return false;
>> +    }
>> +  }
>> +
>>
>> I think you can move that definition in SelectionDAGNodes.h. You can
>> simplify the code in method 'BinaryWithFlagsSDNode::classof' If you
>> can make that definition visible. For example, method '
>> BinaryWithFlagsSDNode ::classof'  could be rewritten as follows:
>>
>> //
>>   static bool classof(const SDNode *N) {
>>     return isBinOpWithFlags(N->getOpcode());
>> //
>>
>> Also, if possible, please remove macro BINOP_NODES_W_FLAGS. In my
>> opinion it doesn't make the code more readable.
>
>
> About that ... BINOP_NODES_W_FLAGS is used in a couple of places (in
> switches) where mainly I cannot avoid to have the case labels there (I would
> like to use isBinOpWithFlags() everywhere, but in switches is not possible
> and putting the check outside the switch would be less performant, so I
> prefer having the cases there.
>
> Do you think just duplicating the cases in those couple of places would be
> ok?

Yes, that was the idea. I don't think it is bad to have those cases
duplicated in 'AddNodeIDCustom' and 'isBinOpWithFlags' . We already do
something similar for other dag nodes (see for example AtomicSDNode).
That said, your approach is not wrong. It is just that, in general, I
prefer not to use macros when possible.