[PATCH] D91255: [AArch64] Rearrange (dup(sext/zext)) to (sext/zext(dup))

[Nicholas Guy via Phabricator via llvm-commits]

NickGuy added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:10584-10586
+
+      // TODO Why can this sometimes fail?
+      if(isa<Instruction>(&Shuffle->getOperandUse(0)))
----------------
SjoerdMeijer wrote:
> NickGuy wrote:
> > SjoerdMeijer wrote:
> > > NickGuy wrote:
> > > > In a number of tests, this line is hit more than a few times. Only 1 of which it fails on. Using .dump() to try and identify why didn't help, as it appears to be the same style as the others that pass (e.g. %tmp3 = sext <8 x i8> %arg to <8 x i16>)
> > > > If anyone can provide insight to this, I would greatly appreciate it :)
> > > what do you mean by fail? I guess that's a segfault?
> > > I guess you need to make sure it is an instruction first with dyn_cast, then you check its operands and uses. 
> > Sorry, I should've been a bit clearer.
> > It fails with an assertion error that stems from an invalid cast in CodeGenPrepare::tryToSinkFreeOperands.
> > 
> > Shuffle->getOperandUse(0).dump() outputs something that looks like an instruction, but reportedly isn't, and only in a single case. It's simple to work around, but I was curious as to if anyone had an idea as to what was happening.
> Still a bit unclear to me.
> But are you actually checking Shuffle is an instruction? I also don't see what the workaround is.
Shuffle is always an instruction at this point, the part that's causing the assertion failure is pushing the operand use into Ops. The result of Shuffle->getOperandUse(0) is what is sometimes not an instruction.
The workaround is the isa<Instruction>(&Shuffle->getOperandUse(0) check.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14987
 
+static SDValue performDUPSextCombine(SDNode *N, SelectionDAG &DAG, bool Signed) {
+  auto Operand = N->getOperand(0);
----------------
SjoerdMeijer wrote:
> Nit: performDUPSextCombine -> performDUPSExtCombine
I opted for performDUPExtCombine, as it performs both signed and zero extends


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14988
+static SDValue performDUPSextCombine(SDNode *N, SelectionDAG &DAG, bool Signed) {
+  auto Operand = N->getOperand(0);
+  auto ExtOperand = Operand.getOperand(0);
----------------
SjoerdMeijer wrote:
> Nit: think the coding style is to use:
> 
>   auto *Operand = ...
> 
> when it is a pointer.
> 
> Same for more declarations below.
None of these are pointer types, though that's good to know for future :)


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14994
+  // Cannot operate on non-vector dups
+  if(!NType.isVector())
+    return SDValue();
----------------
SjoerdMeijer wrote:
> Can you run clang-format on your patch? I've tried reading this function, but all these lint message makes it pretty unreadable to me.
Done, sorry about that


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91255