[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