[PATCH] D104868: [ARM] Fix crash in chained BFI combine due to incorrectly RAUW'ing a node.

[Amara Emerson via Phabricator via llvm-commits]

aemerson added inline comments.


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:14049
+          DCI.DAG.getConstant(NewFromMask.countTrailingZeros(), dl, VT));
+    return DCI.DAG.getNode(ARMISD::BFI, dl, VT, CombineBFI.getOperand(0), From1,
                            DCI.DAG.getConstant(~NewToMask, dl, VT));
----------------
efriedma wrote:
> Hang on, I'm not sure this is right.  At least, it's not obviously equivalent.  FindBFIToCombineWith can look through multiple BFI operations.  Not sure if we have proper test coverage for this.
Right, looks like FindBFIToCombineWith tries to skip unrelated BFIs, and the one it can combine with is CombineBFI. I still think this change is correct, but there's not enough test coverage here.

Having looked further, I can't actually force it to generate chains of more than 2 BFIs at all, since this DAG combine ends up running in between each attempt, combining them before 3 chained BFIs can exist at all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104868