[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