[PATCH] D18802: Improve support for i386 and i486 CPUs.

[James Y Knight via llvm-commits]

jyknight added inline comments.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:96
@@ -96,1 +95,3 @@
+    else if (Subtarget.has486Insns())
+      setMaxAtomicSizeInBitsSupported(32); // has cmpxchg
   }
----------------
echristo wrote:
> jyknight wrote:
> > RKSimon wrote:
> > > Subtarget.has486Insns() and Subtarget.has586Insns() are quiet vague about actual features, would it not be clearer to instead add Subtarget.hasCMPXCHG8() and Subtarget.hasCMPXCHG() that then uses Has586/Has486 internally?
> > > 
> > > Same for BSWAP (Subtarget.hasBSWAP()) below.
> > Sure, sounds fine.
> Couple of comments here:
> 
> a) I definitely like the choice of better function naming for whether or not we have a particular cpu feature.
> b) I'm not sure whether or not we want to conditionalize the 486/586 insns on sets of subtarget features or a broad ISA level feature. Unlike most of the later instructions added there's very little "subsetting" that we can do for this, that said, it forms a contrast with the rest of the port where we do have subtarget features for everything not baseline pentium.
> c) That leads us to "hey, why not just set pentium as the baseline set of features and stop letting people select 386/486 as part of their compiles". Any thoughts here? 
My thoughts:

I have no personal use for < 586 processors. I have no idea if anyone in the world actually needs it. Quite possibly they don't. (I really have no idea).

However, it seems unfortunate that llvm/clang sort of pretend to support them, but don't actually. It would probably be better to return an error for -march=i[34]86, if we aren't going to support it. Although, doing that might break people's build setups, who decide to compile with -march=i386 for whatever reason, even though they don't actually mean to.

The code to support the processors is almost trivial, so there's really little reason not to just do it. (That's the only reason I even sent this patch -- because it touches upon the atomics stuff I'm working on, that there were FIXMEs about this support, and it looked fairly trivial to fix)


http://reviews.llvm.org/D18802