rjmccall added a comment.

Do we really consider the existing atomic intrinsics to not impose added 
alignment restrictions?  I'm somewhat concerned that we're going to produce IR 
here that's either really suboptimal or, worse, unimplementable, just because 
we over-interpreted some cue about alignment.  I guess it would only be a 
significant problem on a target where types are frequently under-aligned for 
what atomics need, which is not typical, or when the user is doing atomics on a 
field of something like a packed struct.



================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:354
   llvm::Type *Int128PtrTy = Int128Ty->getPointerTo();
-  Destination = CGF.Builder.CreateBitCast(Destination, Int128PtrTy);
-  Address ComparandResult(CGF.Builder.CreateBitCast(ComparandPtr, Int128PtrTy),
-                          CGF.getContext().toCharUnitsFromBits(128));
+  DestAddr = CGF.Builder.CreateBitCast(DestAddr, Int128PtrTy);
+  ComparandAddr = CGF.Builder.CreateBitCast(ComparandAddr, Int128PtrTy);
----------------
Since you're changing this code anyway, please make this do 
`CreateElementBitCast(DestAddr, Int128Ty)` so that it's address-space-correct.

There are a lot of other lines in the patch that would benefit from the same 
thing.


================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:3765
     Ptr = Builder.CreateBitCast(Ptr, Int8Ty->getPointerTo(AddrSpace));
+    Address PtrAddr(Ptr, CharUnits::One());
     Value *NewVal = Builder.getInt8(1);
----------------
This should be using `EmitPointerWithAlignment` instead of assuming an 
alignment of 1.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97224

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to