[PATCH] Passing down BinaryOperator flags to BinarySDNode + X86 optimization
Marcello Maggioni
hayarms at gmail.com
Wed Jun 4 00:46:21 PDT 2014
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/3a8cce12/attachment.html>
More information about the llvm-commits
mailing list