[PATCH] D29542: [TargetInfo] Adjust x86-32 atomic support to the CPU used

[Michał Górny via Phabricator via cfe-commits]

mgorny added a comment.

In https://reviews.llvm.org/D29542#666831, @jlebar wrote:

> > Could someone help me figure out what is the cause and correct solution to that failure? @jlebar?
>
> You can see in NVPTXTargetInfo that we read properties from the host targetinfo so that we export the same macros.  The problem here seems to be that we're mutating the x86 targetinfo after the nvptx targetinfo reads its properties.
>
> Does that give you enough context to fix the problem?


Thanks. I'll try to find a reasonably sane solution ;-).



================
Comment at: lib/Basic/Targets.cpp:4244
+    } else // allow locked atomics up to 4 bytes
+      MaxAtomicPromoteWidth = 32;
+  }
----------------
dim wrote:
> Are you purposefully not setting `MaxAtomicInlineWidth` here?  (It seems from `TargetInfo` that the default value is zero.)
> 
Yes. I've based this on what's done for ARM. Unless I misunderstood something, this means that on 'plain' i386 there is no inline atomics support but we still want to do atomics-via-locking up to 32-bit types. I'm not sure about 32/64 here to match i486.


================
Comment at: lib/Basic/Targets.cpp:4265
 
-    // x86-32 has atomics up to 8 bytes
-    // FIXME: Check that we actually have cmpxchg8b before setting
-    // MaxAtomicInlineWidth. (cmpxchg8b is an i586 instruction.)
-    MaxAtomicPromoteWidth = MaxAtomicInlineWidth = 64;
+    setAtomic();
   }
----------------
dim wrote:
> As far as I can see, in the constructor this call is _always_ made with `CPU` set to `CK_Generic`, i.e. zero.  Therefore, the "allow locked atomics up to 4 bytes" path in `setAtomic` is always chosen.  Maybe it is clearer to just initialize `MaxAtomicPromoteWidth` to 32 directly here, instead?
> 
Well, I just copied the idea from ARM. I thought of it more like 'make sure it is initialized to some value, possibly update it later when setting CPU'. I'm fine either way.


Repository:
  rL LLVM

https://reviews.llvm.org/D29542