[PATCH] D15471: [IR] Add support for floating pointer and vector atomic loads and stores

[Sanjoy Das via llvm-commits]

sanjoy added a subscriber: sanjoy.
sanjoy added a comment.

Minor drop by comments inline.


================
Comment at: docs/LangRef.rst:6994
@@ -6993,3 +6993,3 @@
 when they may see multiple atomic stores. The type of the pointee must
-be an integer type whose bit width is a power of two greater than or
-equal to eight and less than or equal to a target-specific size limit.
+be an type whose bit width is a power of two greater than or equal to 
+eight and less than or equal to a target-specific size limit.
----------------
Minor & optional language suggestion:  right now this can be read as "is >= 2^8".  Also, I'm not sure if the "greater than or equal to a target specific limit" is better than "greater than a target specific limit".

Might be clearer to phrase this as:

... be a type whose bit width is a power of two, is not less than 8, and is greater than a target specific limit ...


================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:199
@@ +198,3 @@
+/// Get the iX type with the same bitwidth as T.
+Type *AtomicExpand::getCorrespondingIntegerType(Type *T, const DataLayout &DL) {
+  EVT VT = TLI->getValueType(DL, T);
----------------
Why not have `IntegerType *` as the return type?

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:209
@@ +208,3 @@
+LoadInst *AtomicExpand::convertAtomicLoadToIntegerType(LoadInst *LI) {
+  auto *F = LI->getParent()->getParent()->getParent();
+  Type *NewTy = getCorrespondingIntegerType(LI->getType(),
----------------
`F` is a `llvm::Module`, so please call it `M`.  Also, there is an `Instruction::getModule`.

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:218
@@ +217,3 @@
+                              Addr->getType()->getPointerAddressSpace());
+  Value *NewAddr= Builder.CreateBitCast(Addr, PT);
+  
----------------
Very minor: spacing before `=`.

================
Comment at: lib/CodeGen/AtomicExpandPass.cpp:292
@@ +291,3 @@
+  IRBuilder<> Builder(SI);
+  auto *F = SI->getParent()->getParent()->getParent();
+  Type *NewTy = getCorrespondingIntegerType(SI->getValueOperand()->getType(),
----------------
Again, I think `auto *M = SI->getModule()` is better.


http://reviews.llvm.org/D15471