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

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:10577
+
+    for (auto OpIdx : enumerate(I->operands())) {
+      Instruction *Op = dyn_cast<Instruction>(OpIdx.value().get());
----------------
Nit: do you need the enumerate? Can you do just:

  for (auto *OpIdx : I->operands())


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:10584-10586
+
+      // TODO Why can this sometimes fail?
+      if(isa<Instruction>(&Shuffle->getOperandUse(0)))
----------------
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.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14986
 }
 
+static SDValue performDUPSextCombine(SDNode *N, SelectionDAG &DAG, bool Signed) {
----------------
Nit: A comment on what we are exactly combining here.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14987
 
+static SDValue performDUPSextCombine(SDNode *N, SelectionDAG &DAG, bool Signed) {
+  auto Operand = N->getOperand(0);
----------------
Nit: performDUPSextCombine -> performDUPSExtCombine


================
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);
----------------
Nit: think the coding style is to use:

  auto *Operand = ...

when it is a pointer.

Same for more declarations below.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:14994
+  // Cannot operate on non-vector dups
+  if(!NType.isVector())
+    return SDValue();
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91255