[PATCH v2 02/14] [x86] Add basic support for .code16

[David Woodhouse]

On Sun, 2014-01-05 at 17:17 -0600, Craig Topper wrote:
> Comments for the 3 feature flag version of the patch
> 
> 
> +def In32BitMode  : Predicate<"!Subtarget->is32Bit()">,
> +                             AssemblerPredicate<"Mode32Bit",
> +                            "32-bit mode">;
> 
> 
> This Predicate condition is inverted

Hm, and never used? A grep for 'is64Bit' shows lots of matches in *.inc,
but there are no matches for is32Bit or is16Bit.

> Nothing ever sets In32BitMode in the Subtarget

As with In64BitMode, I believe it's done by
llvm::X86Subtarget::ParseSubtargetFeatures() in X86GenSubtargetInfo.inc:

  if ((Bits & X86::Mode16Bit) != 0) In16BitMode = true;
  if ((Bits & X86::Mode32Bit) != 0) In32BitMode = true;
  if ((Bits & X86::Mode64Bit) != 0) In64BitMode = true;

I don't see anything else that sets In64BitMode... did I miss something?


> Can we avoid passing Features through all the functions until the
> relaxed instruction patch is reviewed?

Hm, I suppose so. Although fairly much *all* the proposed solutions to
PR18303 involve passing the correct set of feature bits in, so that's
going to be necessary. But I can take it back out again, sure.

> 
> +  } else if (is64BitMode(Features)) {
>     ...
> +  } else if (is16BitMode(Features)) {
> +   ...
> +  } else if (is32BitMode(Features)) {
>     ...
>    } else {
> 
> 
> Isn't the code inside the final else block unreachable now?

Yes. Removed; thanks.

> 
> 
> 
> +  else
> +    ToggleFeature(X86::Mode32Bit);
> 
> 
> This should be based off of In32BitMode shouldn't it? Once its
> initialization problem is fixed.

Well, since I haven't shown anyone the patches that add -m16 mode and an
x86-16 target, being in the '!In64BitMode' else clause was equally
correct. But sure, I can make it if (In32BitMode) too.

I couldn't work out how to exercise this code. I *did* suffer issues
with Mode32Bit being uninitialised, but that was actually fixed in all
cases by this hunk in lib/Target/X86/MCTargetDesc/X86MCTargetDesc.cpp:

@@ -47,9 +47,9 @@ std::string X86_MC::ParseX86Triple(StringRef TT) {
   Triple TheTriple(TT);
   std::string FS;
   if (TheTriple.getArch() == Triple::x86_64)
-    FS = "+64bit-mode";
+    FS = "+64bit-mode,-32bit-mode";
   else
-    FS = "-64bit-mode";
+    FS = "-64bit-mode,+32bit-mode";
   return FS;
 }
 

And yeah, eventually we may want to check for Triple::x86 there because
it just *might* be Triple::x86_16. But as I said, I haven't shown anyone
those patches yet..., so 'else' is fine for now ;)

Will rebuild the tree according to the above suggestions, retest and
push it out again. Thanks.

-- 
dwmw2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5745 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140106/27a9a361/attachment.bin>