[PATCH] D20019: [PPC] exploitation of new xscmp*, as well as xsmaxcdp and xsmincdp

[Nemanja Ivanovic via llvm-commits]

nemanjai added inline comments.

================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:498
@@ +497,3 @@
+
+    switch (CC) {
+       case ISD::CondCode::SETNE:
----------------
amehsan wrote:
> nemanjai wrote:
> > Although the fall-through seems reasonable here, I think it's a good idea to add comments to that end. I'm not sure if everyone will agree with me though. So maybe others can chime in here as well.
> Are you in doubt about functional correctness of fall-through or something else? functional correctness should be covered in the testcases. I will think about this again, to see if there are missing patterns in the testcases.
No, the fall-through paths certainly seem fine. I'm only suggesting that fall-through occurrences in switch statements should be commented to inform the reader that this was intent rather than careless omission. I don't think it's even necessary to justify the fall-through (that should be left to the reader), just something as simple as
  // fall-through

================
Comment at: lib/Target/PowerPC/PPCISelDAGToDAG.cpp:528
@@ +527,3 @@
+                             (N->getOperand(0).getOperand(1).
+                                               getOperand(2))->get();
+
----------------
amehsan wrote:
> nemanjai wrote:
> > Is it impossible that these operands do not exist? Namely, is it not possible that operand 1 of N does not have 3 operands thereby causing this call to assert for trying to get an invalid operand? Both here and below.
> N has an opernad(0) because it is a select_cc. we have checked that N->getOperand(0).getOpcode() is and ISD::AND so it has operand (1). and we have checked that both N->getOperand(0).getOperand(0) and N->getOperand(0).getOperand(1) are SETCC so it has operand 0, 1 and 2.
OK, excellent. I just didn't look through the early exit out of this conditional branch in detail. Although that brings me to a point I was initially going to post about the if statement above. I find that a descriptive comment for such involved conditional statements is invaluable.

Overall, it might be nice for this function to have a comment at the top describing all the kinds of DAGs it handles. I understand that we don't comment every possible DAG combine, but when the logic is not easy and straightforward to follow by reading the code, I find a descriptive comment goes a long way for readability.


http://reviews.llvm.org/D20019