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

[Craig Topper]

Looking again, I think In16BitMode in the subtarget is also unitialized. It
needs to be set to false by the X86Subtarget constructor.


On Sun, Jan 5, 2014 at 5:17 PM, Craig Topper <craig.topper at gmail.com> 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
>
>
> 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
>



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