[PATCH v2 02/14] [x86] Add basic support for .code16
David Woodhouse
dwmw2 at infradead.org
Sun Jan 5 16:09:44 PST 2014
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>
More information about the llvm-commits
mailing list