submit [PATCH] SimplifyCFG for code review

Ye, Mei Mei.Ye at amd.com
Tue Jun 18 15:10:54 PDT 2013


Hi Sean

Thanks for your time.  The .ll are generated from attached .cpp.  It is a simple case.
I don't mean the comments in the implementation to be doxygen-generated, but I borrow the "\param" wording.   Sorry for the confusion.
The comments in the class declaration is to be doxygen-generated.  Thank you for suggestion using clang-format.
Some format problem is likely due to file copies between my linux and window machine.  I will double check.


-Mei


From: Sean Silva [mailto:silvas at purdue.edu]
Sent: Tuesday, June 18, 2013 2:58 PM
To: Ye, Mei
Cc: llvm-commits at cs.uiuc.edu
Subject: Re: submit [PATCH] SimplifyCFG for code review

Those regression tests seem really large. Is there any way you could reduce them?

+/// If \param [in] BB has more than one predecessor that is a conditional
+/// branch, attempt to use parallel and/or for the branch condtion. \returns
+/// true on success.

Spelling: s/condtion/condition/.
Also, typically doxygen commands are put on a line of their own instead of inline with prose (I'm not sure that doxygen even accepts them inline like you have them here).

+/// Before:
+///   ......
+///   %cmp10 = fcmp une float %tmp1, %tmp2
+///   br i1 %cmp1, label %if.then, label %lor.rhs

You want \code + \endcode <http://www.stack.nl/~dimitri/doxygen/manual/commands.html#cmdcode> enclosing this code display, so that doxygen will format it correctly.

+      if (UCond || !PP || (Preds.count(PP) == 0) ||
+`        (PTI->getNumSuccessors() != 1) ||
+`        Pred->hasAddressTaken())
+        return false;

Please don't use hard tabs. Your patch also introduces a bunch of trailing whitespace. Also, there are a number of formatting issues; I recommend using clang-format on the regions of code that you are adding to ensure they comply with our preferred code formatting.

-- Sean Silva
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130618/84ca315a/attachment.html>


More information about the llvm-commits mailing list