[PATCH] D64616: [ARM][LowOverheadLoops] Fix branch target codegen

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:12997
+// (brcond (setcc (loop.decrement), 0, ne), header)
+static SDValue IsLoopIntrinsic(SDValue N, ISD::CondCode &CC, int &Imm,
+                               bool &Negate) {
----------------
nit: `IsLoopIntrinsic` is suggesting it is returning a bool, perhaps `searchLoopIntrinsic`


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:13038
 
-  if ((Opc == ISD::XOR || Opc == ISD::SETCC) &&
-      (CC->getOperand(0)->getOpcode() == ISD::INTRINSIC_W_CHAIN)) {
+  // The hwloop intrinsics that we're interested are used for control-flow,
+  // either for entering or exiting the loop:
----------------
nit: interested -> interested in?


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:13045
+  //   the loop.
+  // So here, we need to check that how the brcond is using the result of each
+  // of the intrinsics to ensure that we're branching to the right place at the
----------------
nit: "... check that how ..." -> "check how"?


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:13076
 
-  unsigned IntOp = cast<ConstantSDNode>(Int.getOperand(1))->getZExtValue();
-  if (IntOp != Intrinsic::test_set_loop_iterations)
-    return SDValue();
+  assert((CC == ISD::SETEQ || CC == ISD::SETNE) &&
+         "unexpected condition code");
----------------
Silly question: can you remind me why we are only looking at EQ and NE? 
In tests only these 2 conditions are used. Does it make sense to test others as well, see what happens?


================
Comment at: lib/Target/ARM/ARMISelLowering.h:129
       WLS,          // Low-overhead loops, While Loop Start
+      LoopDec,      // Really a part of LE, performs a sub.
+      LE,           // Low-overhead loops, Loop End
----------------
nit: all capitals for consistency?


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

https://reviews.llvm.org/D64616