[PATCH] D39001: [ARM] Or combine to BFI function

[John Brawn via Phabricator via llvm-commits]

john.brawn added inline comments.


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:10402
 
       // Do not add new nodes to DAG combiner worklist.
       DCI.CombineTo(N, Res, false);
----------------
You're now returning N in an SDValue and that gets returned from PerformORCombine, so now we will be adding the new nodes to the DAG combiner worklist. Is this intentional? Either way something needs to be adjusted here (and in the other places where this comment occurs).


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:10557-10559
+  // BFI is only available on V6T2+
+  if (Subtarget->isThumb1Only() || !Subtarget->hasV6T2Ops())
+    return SDValue();
----------------
It would be better to do this in PerformORCombineToBFI (in general less preconditions is better).


https://reviews.llvm.org/D39001