[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