[PATCH] Passing down BinaryOperator flags to BinarySDNode + X86 optimization
Daniil Troshkov
troshkovdanil at gmail.com
Tue Jun 3 07:46:06 PDT 2014
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 fixed some style problems.
>>> - Integrated the changes Pete pointed out:
>>> - Added the flags to the NodeID construction
>>> - Created a test case for the CSE (not sure it is the best test though
>>> ... I'm not an expert testing this kind of stuff)
>>> - Merged the test for the performance optimization in the optimization
>>> patch.
>>> - Integrated the code cleanups that were specified.
>>> - Reduced the test case for the x86 opt.
>>>
>>> Now as a result there are only two patches:
>>> - flag_nodes.patch
>>> - x86_opt_wrap.patch
>>>
>>> I didn't changed BinarySDNode to have classof(), because I think is
>>> probably better doing it separately , but also because it is kind of more
>>> complex than it seems , because it is not clear exactly what are the
>>> opcodes that become BinarySDNodes (a part from the obvious binary ADD, SUB
>>> ... etc operations).
>>> It is probably necessary to take more time to collect an accurate list
>>> of all the opcodes that always end up being lowered as BinarySDNodes and
>>> add the classof() then.
>>>
>>> Marcello
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> <https://e.mail.ru/compose?To=llvm%2dcommits@cs.uiuc.edu>
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>>>
>>>
>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140603/97cb4a81/attachment.html>
More information about the llvm-commits
mailing list