<div dir="ltr">Thanks,<div><br></div><div>I agree that this kind of check would be better in the verifier. <span style="line-height:normal">I plan on posting a patch that performs the checks in the verifier soon, and then I will be able to remove the checks from the MIR parser. </span></div><div><br></div><div>Alex</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">2015-07-07 11:15 GMT-07:00 Quentin Colombet <span dir="ltr"><<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br>
> On Jun 27, 2015, at 8:27 AM, Duncan P. N. Exon Smith <<a href="mailto:dexonsmith@apple.com">dexonsmith@apple.com</a>> wrote:<br>
><br>
><br>
>> On 2015 Jun 26, at 15:34, Alex Lorenz <<a href="mailto:arphaman@gmail.com">arphaman@gmail.com</a>> wrote:<br>
>><br>
>> Hi dexonsmith, bob.wilson, bogner,<br>
>><br>
><br>
> +quentin<br>
><br>
>> This patch is based on a previous serialization patch that serializes implicit register flags (<a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D10709&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=7UZ8exiOkNIoopTE4UamwfnvAOd2zzk4eHInsrI-qFg&s=8UvORIWiTYV8I6oYr3skc0P9AKS5geEr8fYLLa-rTiE&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/D10709</a>).<br>
>><br>
>> This patch verifies that the parsed machine instructions have implicit register operands as specified by MCInstrDesc. Variadic and call instructions aren't verified.<br>
>><br>
><br>
> I'm not sure this is the right long term direction.  This seems better:<br>
><br>
> 1. Implicitly add the `-machineverifier` pass to the beginning of the<br>
>    pass pipeline.  Alternatively, create a free function that calls the<br>
>    machine verifier right after parsing MIR (much like `verifyModule()`<br>
>    at the IR-level).<br>
> 2. Assuming these checks are valid (are instructions *incorrect* if they<br>
>    list the wrong implicit flags) and missing from the machine verifier,<br>
>    add them there.<br>
><br>
> Quentin, what do you think?<br>
<br>
</span>I agree that we should check in the verifier that we have the implicit operands defined by the MC description. Now, for instruction that list “wrong” implicit flags, I guess it depends how wrong that is. E.g., it is valid, though redundant, to have an implicit use of a sub register of a flag that is already a use (or implicit use).<br>
<br>
The question is more likely, what do you want to catch here?<br>
<br>
FWIW, I would prefer to have the check only in the verifier. I do not think the problem will occur often enough to have a location to find what is wrong.<br>
<br>
Cheers,<br>
-Quentin<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
><br>
> Regardless of the long term, this obviously is useful in its current form<br>
> for bootstrapping MIR serialization.  We can move/delete/change it later.<br>
> I say commit it with a well-placed:<br>
><br>
>    // FIXME: Move to the -machineverifier.<br>
><br>
> LGTM with that.<br>
><br>
> BTW, most of the review I'm doing here could just as easily be done<br>
> post-commit, especially given that you basically own MIR serialization ;).<br>
><br>
>> REPOSITORY<br>
>> rL LLVM<br>
>><br>
>> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D10781&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=7UZ8exiOkNIoopTE4UamwfnvAOd2zzk4eHInsrI-qFg&s=nDdbIdmOqUgnXhfTomFO2-gmfMbuZeeen6OALUlYhyA&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/D10781</a><br>
>><br>
>> Files:<br>
>> lib/CodeGen/MIRParser/MIParser.cpp<br>
>> test/CodeGen/MIR/X86/expected-different-implicit-operand.mir<br>
>> test/CodeGen/MIR/X86/expected-different-implicit-register-flag.mir<br>
>> test/CodeGen/MIR/X86/expected-number-after-bb.mir<br>
>> test/CodeGen/MIR/X86/global-value-operands.mir<br>
>> test/CodeGen/MIR/X86/large-index-number-error.mir<br>
>> test/CodeGen/MIR/X86/machine-basic-block-operands.mir<br>
>> test/CodeGen/MIR/X86/machine-instructions.mir<br>
>> test/CodeGen/MIR/X86/missing-implicit-operand.mir<br>
>> test/CodeGen/MIR/X86/named-registers.mir<br>
>> test/CodeGen/MIR/X86/register-mask-operands.mir<br>
>> test/CodeGen/MIR/X86/unknown-machine-basic-block.mir<br>
>> test/CodeGen/MIR/X86/unknown-named-machine-basic-block.mir<br>
>><br>
>> EMAIL PREFERENCES<br>
>> <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_settings_panel_emailpreferences_&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=7UZ8exiOkNIoopTE4UamwfnvAOd2zzk4eHInsrI-qFg&s=1malUK1WEQwPAx3noLEoT0ZWRxaz46RHOoB5iUDSdyY&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
>> <D10781.28604.patch><br>
><br>
<br>
</div></div></blockquote></div><br></div>