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

[Alex L]

Thanks,

I agree that this kind of check would be better in the verifier. 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.

Alex


2015-07-07 11:15 GMT-07:00 Quentin Colombet <qcolombet at apple.com>:

>
> > 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>
> >
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150707/f96d81d9/attachment.html>