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

[Eli Friedman via Phabricator via llvm-commits]

efriedma 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));
----------------
aemerson wrote:
> 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.
If you want to change the code so it can't skip unrelated BFIs, that's probably fine.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D104868