[PATCH] MIR Parser: verify the implicit register operands.

[Quentin Colombet]

> On Jun 27, 2015, at 8:27 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
> 
> 
>> On 2015 Jun 26, at 15:34, Alex Lorenz <arphaman at gmail.com> wrote:
>> 
>> Hi dexonsmith, bob.wilson, bogner,
>> 
> 
> +quentin
> 
>> This patch is based on a previous serialization patch that serializes implicit register flags (http://reviews.llvm.org/D10709).
>> 
>> This patch verifies that the parsed machine instructions have implicit register operands as specified by MCInstrDesc. Variadic and call instructions aren't verified.
>> 
> 
> I'm not sure this is the right long term direction.  This seems better:
> 
> 1. Implicitly add the `-machineverifier` pass to the beginning of the
>    pass pipeline.  Alternatively, create a free function that calls the
>    machine verifier right after parsing MIR (much like `verifyModule()`
>    at the IR-level).
> 2. Assuming these checks are valid (are instructions *incorrect* if they
>    list the wrong implicit flags) and missing from the machine verifier,
>    add them there.
> 
> Quentin, what do you think?

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).

The question is more likely, what do you want to catch here?

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.

Cheers,
-Quentin


> 
> Regardless of the long term, this obviously is useful in its current form
> for bootstrapping MIR serialization.  We can move/delete/change it later.
> I say commit it with a well-placed:
> 
>    // FIXME: Move to the -machineverifier.
> 
> LGTM with that.
> 
> BTW, most of the review I'm doing here could just as easily be done
> post-commit, especially given that you basically own MIR serialization ;).
> 
>> REPOSITORY
>> rL LLVM
>> 
>> http://reviews.llvm.org/D10781
>> 
>> Files:
>> lib/CodeGen/MIRParser/MIParser.cpp
>> test/CodeGen/MIR/X86/expected-different-implicit-operand.mir
>> test/CodeGen/MIR/X86/expected-different-implicit-register-flag.mir
>> test/CodeGen/MIR/X86/expected-number-after-bb.mir
>> test/CodeGen/MIR/X86/global-value-operands.mir
>> test/CodeGen/MIR/X86/large-index-number-error.mir
>> test/CodeGen/MIR/X86/machine-basic-block-operands.mir
>> test/CodeGen/MIR/X86/machine-instructions.mir
>> test/CodeGen/MIR/X86/missing-implicit-operand.mir
>> test/CodeGen/MIR/X86/named-registers.mir
>> test/CodeGen/MIR/X86/register-mask-operands.mir
>> test/CodeGen/MIR/X86/unknown-machine-basic-block.mir
>> test/CodeGen/MIR/X86/unknown-named-machine-basic-block.mir
>> 
>> EMAIL PREFERENCES
>> http://reviews.llvm.org/settings/panel/emailpreferences/
>> <D10781.28604.patch>
>