<div dir="ltr">Comments for the 3 feature flag version of the patch<div><br></div><div><div>+def In32BitMode : Predicate<"!Subtarget->is32Bit()">,</div><div>+ AssemblerPredicate<"Mode32Bit",</div>
<div>+ "32-bit mode">;</div></div><div><br></div><div>This Predicate condition is inverted</div><div><br></div><div><br></div><div>Nothing ever sets In32BitMode in the Subtarget</div>
<div><br></div><div><br></div><div><div>- bool is64BitMode() const {</div><div>- // FIXME: Can tablegen auto-generate this?</div><div>- return (STI.getFeatureBits() & X86::Mode64Bit) != 0;</div><div>+ bool is64BitMode(uint64_t Features) const {</div>
<div>+ return (Features & X86::Mode64Bit) != 0;</div><div> }</div><div> </div><div>- bool is32BitMode() const {</div><div>- // FIXME: Can tablegen auto-generate this?</div><div>- return (STI.getFeatureBits() & X86::Mode64Bit) == 0;</div>
<div>+ bool is32BitMode(uint64_t Features) const {</div><div>+ return (Features & X86::Mode32Bit) != 0;</div><div>+ }</div><div>+</div><div>+ bool is16BitMode(uint64_t Features) const {</div><div>+ return (Features & X86::Mode16Bit) != 0;</div>
<div> }</div></div><div><br></div><div>Can we avoid passing Features through all the functions until the relaxed instruction patch is reviewed?</div><div><br></div><div><br></div><div><br></div><div><div> if (TSFlags & X86II::AdSize) {</div>
<div> need_address_override = true;</div><div> } else if (MemOperand == -1) {</div><div> need_address_override = false;</div><div>- } else if (is64BitMode()) {</div><div>+ } else if (is64BitMode(Features)) {</div>
<div> assert(!Is16BitMemOperand(MI, MemOperand));</div><div> need_address_override = Is32BitMemOperand(MI, MemOperand);</div><div>- } else if (is32BitMode()) {</div><div>+ } else if (is16BitMode(Features)) {</div>
<div>+ assert(!Is64BitMemOperand(MI, MemOperand));</div><div>+ need_address_override = !Is16BitMemOperand(MI, MemOperand);</div><div>+ } else if (is32BitMode(Features)) {</div><div> assert(!Is64BitMemOperand(MI, MemOperand));</div>
<div> need_address_override = Is16BitMemOperand(MI, MemOperand);</div><div> } else {</div></div><div><br></div><div>Isn't the code inside the final else block unreachable now?</div><div><br></div><div><br></div>
<div><div>+ else</div><div>+ ToggleFeature(X86::Mode32Bit);</div></div><div><br></div><div>This should be based off of In32BitMode shouldn't it? Once its initialization problem is fixed.</div><div><br></div></div>
<div class="gmail_extra"><br><br><div class="gmail_quote">On Sun, Jan 5, 2014 at 4:36 PM, Craig Topper <span dir="ltr"><<a href="mailto:craig.topper@gmail.com" target="_blank">craig.topper@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">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.<div>
<br></div><div>Anyway for now I guess the least ugly is to use 3 feature bits.</div>
<div class="gmail_extra"><div><div class="h5"><br><br><div class="gmail_quote">On Sun, Jan 5, 2014 at 4:01 PM, Joerg Sonnenberger <span dir="ltr"><<a href="mailto:joerg@britannica.bec.de" target="_blank">joerg@britannica.bec.de</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div>On Sun, Jan 05, 2014 at 09:57:21PM +0000, David Woodhouse wrote:<br>
> On Sun, 2014-01-05 at 22:51 +0100, Joerg Sonnenberger wrote:<br>
> > At the moment, In64BitMode is a toogle between two valid modes.<br>
> > Without copying things around in random places (like to the<br>
> > fragments), that's somewhat sane. But the more places are dealing with<br>
> > feature bits, the more important it becomes to ensure the correctness.<br>
> > Does that make sense?<br>
><br>
> Yeah, but the correctness aspect is simply and succinctly stated as<br>
> "Mode16Bit and Mode64Bit are mutually exclusive" — that is, the value<br>
> set {1,1} is invalid.<br>
><br>
> I don't see the merit in inverting them both just to declare that {0,0}<br>
> is invalid instead. Can you show a "likely" coding error or paradigm<br>
> which would make that actually useful?<br>
<br>
</div></div>If the feature bits are not copied in one place, a bad mode is choosen.<br>
Not using {0,0} as invalid encoding prevents exactly that.<br>
<div><div><br>
Joerg<br>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br><br clear="all"><div><br></div></div></div><span class="HOEnZb"><font color="#888888">-- <br>~Craig
</font></span></div></div>
</blockquote></div><br><br clear="all"><div><br></div>-- <br>~Craig
</div>