[PATCH] D77213: Handle part-word LL/SC in atomic expansion pass

[James Y Knight via Phabricator via llvm-commits]

jyknight added a comment.

Functionality seems fine, just some style nits.

Also: please clang-format your patch (e.g. using git-clang-format).



================
Comment at: llvm/lib/CodeGen/AtomicExpandPass.cpp:653
+
+Value *PartwordMaskValues::extract(IRBuilder<> &Builder,
+                                   Value *WideWord) const {
----------------
These two functions appear to be only used in expandAtomicCmpXchg, maybe better to just make them lambdas in it or static helper functions right above it.


================
Comment at: llvm/lib/CodeGen/AtomicExpandPass.cpp:807
 
-  PartwordMaskValues PMV =
-      createMaskInstrs(Builder, AI, AI->getType(), AI->getPointerOperand(),
-                       TLI->getMinCmpXchgSizeInBits() / 8);
+  PartwordMaskValues PMV(Builder, AI, AI->getType(), AI->getPointerOperand(),
+                         TLI->getMinCmpXchgSizeInBits() / 8);
----------------
I'm not fond of the style where constructing an object is used primarily to side-effect one of its arguments. It seems clearer, generally, to use a separate function-call with a meaningful name, rather than an object constructor. (That is: the way the code was before, with a createMaskInstrs function seems better).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77213