[PATCH] D104868: [ARM] Fix crash in chained BFI combine due to incorrectly RAUW'ing a node.
Amara Emerson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Jun 24 13:48:38 PDT 2021
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
More information about the llvm-commits
mailing list