[PATCH] D100304: [AArch64][NEON] Match (or (and -a b) (and (a+1) b)) => bit select

[Joe Ellis via Phabricator via llvm-commits]

joechrisellis added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:12581-12597
+      SDValue O0 = N0->getOperand(i);
+      SDValue O1 = N1->getOperand(j);
+      SDValue Sub, Add, SubSibling, AddSibling;
+
+      // Find a SUB and an ADD operand, one from each AND.
+      if (O0.getOpcode() == ISD::SUB && O1.getOpcode() == ISD::ADD) {
+        Sub = O0;
----------------
bsmith wrote:
> joechrisellis wrote:
> > Small suggestion -- it might be possible to get rid of some duplicate code here by doing something like:
> > 
> > ```
> > if (O0.getOpcode() == ISD::SUB)
> >     std::swap(O0, O1);
> > 
> > if (O1.getOpcode() != ISD::ADD)
> >     continue;
> > 
> > // if we get here, O0 is guaranteed to be the sub, and O1 the add
> > ...
> > ```
> > 
> > If this works, I _think_ it might make things clearer. But I don't know for sure. If it starts to look uglier then I have no problem with it as-is. :) 
> To be honest I think something like this is just going to end up being more confusing, for the sake of saving a couple of duplicated lines, I really don't think it's worth it. Also note that your example doesn't work since the second check isn't correct in the case the operands have been swapped.
You're right! It should have been something like:

```
if (O0.getOpcode() != ISD::SUB)
    std::swap(O0, O1);

if (O0.getOpcode() != ISD::SUB || O1.getOpcode() != ISD::ADD)
    continue;

// if we get here, O0 is guaranteed to be the sub, and O1 the add
...
```

... either way, you are right; `N0` and `N1` make this a bit more awkward than I had anticipated. ACK. 🙂 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D100304/new/

https://reviews.llvm.org/D100304