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

Sam Parker via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Oct 18 03:51:55 PDT 2017


samparker 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);
----------------
john.brawn wrote:
> 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).
Yes, returning an SDValue of the original SDNode tells the combiner than N is now dead. I will update the comment.


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


https://reviews.llvm.org/D39001





More information about the llvm-commits mailing list