FYI, I'm commenting on each individual patch by replying to the email where you attached that patch. So on to #2...<div><br></div><div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
From: Michael Liao <<a href="mailto:michael.hliao@gmail.com">michael.hliao@gmail.com</a>><br>Date: Wed, 25 Jul 2012 17:15:02 -0700<br>Subject: [PATCH 2/6] refine code generation for atomic operations with<br> spin-loop</blockquote>
<div><br></div><div>For commit logs, I would encourage you to write them in a bit more of a prose form. Try to include the motivation or end goal, some amount of context. No need to go crazy with it, just helps the reviewers.</div>
<div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">- replace EmitAtomicMinMaxWithCustomInserter &<br>
EmitAtomicBitwiseWithCustomInserter into a single EmitAtomicLoadArith<br> for all pseudo atomic operations, i.e. atomic operations based on<br> spin-loop or compare-exchange loop</blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
- replace EmitAtomicBit6432WithCustomInserter with<br> EmitAtomicLoadArith6432 for 64-bit atomic operations on 32-bit target<br>- remove unncessary modifiers for pseudo atomic instruction td<br></blockquote><div><br></div>
<div>Can you split these three bullet points into their own three small patches? It looks like none of them change behavior, they're just refactorings.</div><div><br></div><div>Also, when you are fusing routines like this together, please be cautious about growing the functions too large. It is important to factor them heavily. Some comments about this below....</div>
<div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">- reduce one unnecessary load in spin-loop </blockquote>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">- modify existing tests due to spin-loop change</blockquote>
<div><br></div><div>If you can give a quick before/after summary in the change log for this patch, that would be good.</div><div><br></div></div><div><br></div><div>Some comments on the patch itself. I'm going to hold off going into too much detail until after you break it appart into the refactoring patches followed by the functional ones.</div>
<div><br></div><div>+// private utility function<br></div><div><br></div><div>Please, much more and detailed comments. =] This is very tricky code, so try to help future maintainers.</div><div><br></div><div>If possible, use Doxygen-style comments for all of the functions:</div>
<div><br></div><div>/// \brief Utility function to compute the whizz bang widgets.</div><div>///</div><div>/// This function computes whizz bang widgets using the following algorithm:</div><div>/// ...</div><div>///</div>
<div>/// An example of the output:</div><div>/// ...</div><div><br></div><div><div>+ switch (VT.getSimpleVT().SimpleTy) {</div><div>+ default:</div><div>+ llvm_unreachable("Invalid atomic-load-op operand size!");</div>
<div>+ case MVT::i8:</div><div>+ LCMPXCHGOpc = X86::LCMPXCHG8;</div><div>+ LOADOpc = X86::MOV8rm;</div><div>+ break;</div><div>+ case MVT::i16:</div><div>+ LCMPXCHGOpc = X86::LCMPXCHG16;</div><div>+ LOADOpc = X86::MOV16rm;</div>
<div>+ break;</div><div>+ case MVT::i32:</div><div>+ LCMPXCHGOpc = X86::LCMPXCHG32;</div><div>+ LOADOpc = X86::MOV32rm;</div><div>+ break;</div><div>+ case MVT::i64:</div><div>+ LCMPXCHGOpc = X86::LCMPXCHG64;</div>
<div>+ LOADOpc = X86::MOV64rm;</div><div>+ break;</div><div>+ }</div><div><br></div><div>When ever you find yourself with a big switch that just sets variables to values, consider writing a small static helper function that returns the values. Here, I think two functions, one to return the cmxchg variant and one to return the load variant would be better.</div>
</div><div><br></div><div><br></div><div><div>+ unsigned Opc = MI->getOpcode();</div><div>+ switch (Opc) {</div><div>+ default:</div><div>+ llvm_unreachable("Unhandled atomic-load-op opcode!");</div><div>
+ case X86::ATOMAND8:</div><div>+ case X86::ATOMAND16:</div><div>+ case X86::ATOMAND32:</div><div>+ case X86::ATOMAND64:</div><div>+ case X86::ATOMOR8:</div><div>+ case X86::ATOMOR16:</div><div>+ case X86::ATOMOR32:</div>
<div>+ case X86::ATOMOR64:</div><div>+ case X86::ATOMXOR8:</div><div>+ case X86::ATOMXOR16:</div><div>+ case X86::ATOMXOR32:</div><div>+ case X86::ATOMXOR64: {</div><div><br></div><div>Here we have another big switch that should probably be hoisted into a function, but it also has this:</div>
<div><br></div><div>+ unsigned ARITHOpc = (Opc == X86::ATOMAND8) ? X86::AND8rr :</div><div>+ (Opc == X86::ATOMAND16) ? X86::AND16rr :</div><div>+ (Opc == X86::ATOMAND32) ? X86::AND32rr :</div>
<div>+ (Opc == X86::ATOMAND64) ? X86::AND64rr :</div><div>+ (Opc == X86::ATOMOR8) ? X86::OR8rr :</div><div>+ (Opc == X86::ATOMOR16) ? X86::OR16rr :</div>
<div>+ (Opc == X86::ATOMOR32) ? X86::OR32rr :</div><div>+ (Opc == X86::ATOMOR64) ? X86::OR64rr :</div><div>+ (Opc == X86::ATOMXOR8) ? X86::XOR8rr :</div>
<div>+ (Opc == X86::ATOMXOR16) ? X86::XOR16rr :</div><div>+ (Opc == X86::ATOMXOR32) ? X86::XOR32rr :</div><div>+ (Opc == X86::ATOMXOR64) ? X86::XOR64rr :</div>
<div>+ 0;</div><div>+ assert(ARITHOpc && "Invalid atomic-load-op transformation!");</div><div><br></div><div>This is a really hard to read (and terribly inefficient) way to build a switch that produces a value. A function with a switch would seem much clearer.</div>
<div><br></div><div>That said, why re-do a walk over the opcodes? It would seem more obvious to handle this in the one big switch.</div><div><br></div><div>Is there any way to turn this into a table, or better yet, have tablegen pre-compute the mapping from atomfoo -> foo? That would eliminate a *lot* of the boilerplate in this patch.</div>
</div>