[llvm-commits] [PATCH 2/6] revise/enhance atomic primitive code generation

[Chandler Carruth]

FYI, I'm commenting on each individual patch by replying to the email where
you attached that patch. So on to #2...

From: Michael Liao <michael.hliao at gmail.com>
> Date: Wed, 25 Jul 2012 17:15:02 -0700
> Subject: [PATCH 2/6] refine code generation for atomic operations with
>  spin-loop


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.

- replace EmitAtomicMinMaxWithCustomInserter &
>   EmitAtomicBitwiseWithCustomInserter into a single EmitAtomicLoadArith
>   for all pseudo atomic operations, i.e. atomic operations based on
>   spin-loop or compare-exchange loop

- replace EmitAtomicBit6432WithCustomInserter with
>   EmitAtomicLoadArith6432 for 64-bit atomic operations on 32-bit target
> - remove unncessary modifiers for pseudo atomic instruction td
>

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.

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....


> - reduce one unnecessary load in spin-loop

- modify existing tests due to spin-loop change


If you can give a quick before/after summary in the change log for this
patch, that would be good.


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.

+// private utility function

Please, much more and detailed comments. =] This is very tricky code, so
try to help future maintainers.

If possible, use Doxygen-style comments for all of the functions:

/// \brief Utility function to compute the whizz bang widgets.
///
/// This function computes whizz bang widgets using the following algorithm:
/// ...
///
/// An example of the output:
/// ...

+  switch (VT.getSimpleVT().SimpleTy) {
+  default:
+    llvm_unreachable("Invalid atomic-load-op operand size!");
+  case MVT::i8:
+    LCMPXCHGOpc = X86::LCMPXCHG8;
+    LOADOpc = X86::MOV8rm;
+    break;
+  case MVT::i16:
+    LCMPXCHGOpc = X86::LCMPXCHG16;
+    LOADOpc = X86::MOV16rm;
+    break;
+  case MVT::i32:
+    LCMPXCHGOpc = X86::LCMPXCHG32;
+    LOADOpc = X86::MOV32rm;
+    break;
+  case MVT::i64:
+    LCMPXCHGOpc = X86::LCMPXCHG64;
+    LOADOpc = X86::MOV64rm;
+    break;
+  }

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.


+  unsigned Opc = MI->getOpcode();
+  switch (Opc) {
+  default:
+    llvm_unreachable("Unhandled atomic-load-op opcode!");
+  case X86::ATOMAND8:
+  case X86::ATOMAND16:
+  case X86::ATOMAND32:
+  case X86::ATOMAND64:
+  case X86::ATOMOR8:
+  case X86::ATOMOR16:
+  case X86::ATOMOR32:
+  case X86::ATOMOR64:
+  case X86::ATOMXOR8:
+  case X86::ATOMXOR16:
+  case X86::ATOMXOR32:
+  case X86::ATOMXOR64: {

Here we have another big switch that should probably be hoisted into a
function, but it also has this:

+    unsigned ARITHOpc = (Opc == X86::ATOMAND8)  ? X86::AND8rr :
+                        (Opc == X86::ATOMAND16) ? X86::AND16rr :
+                        (Opc == X86::ATOMAND32) ? X86::AND32rr :
+                        (Opc == X86::ATOMAND64) ? X86::AND64rr :
+                        (Opc == X86::ATOMOR8)   ? X86::OR8rr :
+                        (Opc == X86::ATOMOR16)  ? X86::OR16rr :
+                        (Opc == X86::ATOMOR32)  ? X86::OR32rr :
+                        (Opc == X86::ATOMOR64)  ? X86::OR64rr :
+                        (Opc == X86::ATOMXOR8)  ? X86::XOR8rr :
+                        (Opc == X86::ATOMXOR16) ? X86::XOR16rr :
+                        (Opc == X86::ATOMXOR32) ? X86::XOR32rr :
+                        (Opc == X86::ATOMXOR64) ? X86::XOR64rr :
+                        0;
+    assert(ARITHOpc && "Invalid atomic-load-op transformation!");

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.

That said, why re-do a walk over the opcodes? It would seem more obvious to
handle this in the one big switch.

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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120821/75df4942/attachment.html>