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

[Craig Topper]

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


Nothing ever sets In32BitMode in the Subtarget


-  bool is64BitMode() const {
-    // FIXME: Can tablegen auto-generate this?
-    return (STI.getFeatureBits() & X86::Mode64Bit) != 0;
+  bool is64BitMode(uint64_t Features) const {
+    return (Features & X86::Mode64Bit) != 0;
   }

-  bool is32BitMode() const {
-    // FIXME: Can tablegen auto-generate this?
-    return (STI.getFeatureBits() & X86::Mode64Bit) == 0;
+  bool is32BitMode(uint64_t Features) const {
+    return (Features & X86::Mode32Bit) != 0;
+  }
+
+  bool is16BitMode(uint64_t Features) const {
+    return (Features & X86::Mode16Bit) != 0;
   }

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



   if (TSFlags & X86II::AdSize) {
     need_address_override = true;
   } else if (MemOperand == -1) {
     need_address_override = false;
-  } else if (is64BitMode()) {
+  } else if (is64BitMode(Features)) {
     assert(!Is16BitMemOperand(MI, MemOperand));
     need_address_override = Is32BitMemOperand(MI, MemOperand);
-  } else if (is32BitMode()) {
+  } else if (is16BitMode(Features)) {
+    assert(!Is64BitMemOperand(MI, MemOperand));
+    need_address_override = !Is16BitMemOperand(MI, MemOperand);
+  } else if (is32BitMode(Features)) {
     assert(!Is64BitMemOperand(MI, MemOperand));
     need_address_override = Is16BitMemOperand(MI, MemOperand);
   } else {

Isn't the code inside the final else block unreachable now?


+  else
+    ToggleFeature(X86::Mode32Bit);

This should be based off of In32BitMode shouldn't it? Once its
initialization problem is fixed.



On Sun, Jan 5, 2014 at 4:36 PM, Craig Topper <craig.topper at gmail.com> wrote:

> I think this is sort of begging to separate the concept of "assembler
> mode" (which wants to be a numeric encoding) from features which are bit
> oriented.
>
> Anyway for now I guess the least ugly is to use 3 feature bits.
>
>
> On Sun, Jan 5, 2014 at 4:01 PM, Joerg Sonnenberger <
> joerg at britannica.bec.de> wrote:
>
>> On Sun, Jan 05, 2014 at 09:57:21PM +0000, David Woodhouse wrote:
>> > On Sun, 2014-01-05 at 22:51 +0100, Joerg Sonnenberger wrote:
>> > > At the moment, In64BitMode is a toogle between two valid modes.
>> > > Without copying things around in random places (like to the
>> > > fragments), that's somewhat sane. But the more places are dealing with
>> > > feature bits, the more important it becomes to ensure the correctness.
>> > > Does that make sense?
>> >
>> > Yeah, but the correctness aspect is simply and succinctly stated as
>> > "Mode16Bit and Mode64Bit are mutually exclusive" — that is, the value
>> > set {1,1} is invalid.
>> >
>> > I don't see the merit in inverting them both just to declare that {0,0}
>> > is invalid instead. Can you show a "likely" coding error or paradigm
>> > which would make that actually useful?
>>
>> If the feature bits are not copied in one place, a bad mode is choosen.
>> Not using {0,0} as invalid encoding prevents exactly that.
>>
>> Joerg
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>
>
>
> --
> ~Craig
>



-- 
~Craig
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140105/86d2bb3e/attachment.html>