[PATCH] TableGen InstrMapping Bug fix

Thu Dec 1 05:38:27 PST 2016

Yes please, I do not have commit authority.

If you prefer the code snippet you sent earlier, that works for me too. It
looks to me like both solutions would fix the bug (although I didn't test
yours), but yours would provide a few other enhancements as well. Whichever
works, so if you could commit one of them along with the reg test, that
would be great.

Thanks,
Tyler

From:	Vedant Kumar <vsk at apple.com>
To:	Tyler Kenney/Marlborough/IBM at IBMUS
Cc:	llvm-commits at lists.llvm.org
Date:	11/30/2016 06:27 PM
Subject:	Re: [PATCH] TableGen InstrMapping Bug fix
Sent by:	vsk at apple.com

I'm sorry for the delay. This LGTM.

Let me know if you'd like me to commit this for you.

thanks,
vedant

> On Nov 23, 2016, at 3:03 PM, Tyler Kenney <tjkenney at us.ibm.com> wrote:
>
> Hm, I may have forgotten it. I attached it again, but I'll also copy it
below just in case.
>
> Tyler
>
>
> Index: test/TableGen/DuplicateFieldValues.td
> ===================================================================
> --- test/TableGen/DuplicateFieldValues.td (revision 0)
> +++ test/TableGen/DuplicateFieldValues.td (revision 0)
> @@ -0,0 +1,84 @@
> +// RUN: llvm-tblgen -gen-instr-info -I%S/../../include %s | FileCheck %s
> +
> +// CHECK: ABCForm_A
> +// CHECK-NOT: ABCForm_A
> +
> +//
> +// include Target.td for InstrMapping class and define minimally
required objects
> +//
> +
> +include "llvm/Target/Target.td"
> +
> +class DFVReg<string n> : Register<n> {
> + let Namespace = "DFV";
> +}
> +
> +def R0 : DFVReg<"r0">;
> +def DFVRegClass : RegisterClass<"DFV",[i32],0,(add R0)>;
> +def DFVInstrInfo : InstrInfo;
> +
> +def DFVTest : Target {
> + let InstructionSet = DFVInstrInfo;
> +}
> +
> +//
> +// Define a number of a InstrMappings with repeated ValueCol fields
> +//
> +
> +class ABCRel;
> +
> +def getAFormFromBForm : InstrMapping {
> + let FilterClass = "ABCRel";
> + let RowFields = ["BaseName"];
> + let ColFields = ["ABCForm"];
> + let KeyCol = ["B"];
> + let ValueCols = [["A"]];
> +}
> +
> +def getAFormFromCForm : InstrMapping {
> + let FilterClass = "ABCRel";
> + let RowFields = ["BaseName"];
> + let ColFields = ["ABCForm"];
> + let KeyCol = ["C"];
> + let ValueCols = [["A"]];
> +}
> +
> +def getAFormFromDForm : InstrMapping {
> + let FilterClass = "ABCRel";
> + let RowFields = ["BaseName"];
> + let ColFields = ["ABCForm"];
> + let KeyCol = ["D"];
> + let ValueCols = [["A"]];
> +}
> +
> +def getAFormFromEForm : InstrMapping {
> + let FilterClass = "ABCRel";
> + let RowFields = ["BaseName"];
> + let ColFields = ["ABCForm"];
> + let KeyCol = ["E"];
> + let ValueCols = [["A"]];
> +}
> +
> +class I : Instruction {
> + let Namespace = "DFV";
> + let OutOperandList = (outs);
> + let InOperandList = (ins);
> +
> + string BaseName = "";
> + string ABCForm = "";
> +}
> +
> +class isAForm { string ABCForm = "A"; }
> +class isBForm { string ABCForm = "B"; }
> +class isCForm { string ABCForm = "C"; }
> +class isDForm { string ABCForm = "D"; }
> +class isEForm { string ABCForm = "E"; }
> +
> +let BaseName = "0" in {
> + def A0 : I, ABCRel, isAForm;
> + def B0 : I, ABCRel, isBForm;
> + def C0 : I, ABCRel, isCForm;
> + def D0 : I, ABCRel, isDForm;
> + def E0 : I, ABCRel, isEForm;
> +}
> +
> Index: utils/TableGen/CodeGenMapTable.cpp
> ===================================================================
> --- utils/TableGen/CodeGenMapTable.cpp (revision 285805)
> +++ utils/TableGen/CodeGenMapTable.cpp (working copy)
> @@ -542,6 +542,7 @@
> for (unsigned j = i+1; j < FieldValues.size(); j++) {
> if (CurVal == FieldValues[j]) {
> FieldValues.erase(FieldValues.begin()+j);
> + --j;
> }
> }
> }
>
> (See attached file: TableGenInstrMappingDuplicateBugFix.diff)
>
> <graycol.gif>Vedant Kumar ---11/23/2016 05:51:53 PM---> On Nov 22, 2016,
at 12:47 PM, Tyler Kenney <tjkenney at us.ibm.com> wrote: >
>
> From: Vedant Kumar <vsk at apple.com>
> To: Tyler Kenney/Marlborough/IBM at IBMUS
> Cc: llvm-commits at lists.llvm.org
> Date: 11/23/2016 05:51 PM
> Subject: Re: [PATCH] TableGen InstrMapping Bug fix
> Sent by: vsk at apple.com
>
>
>
>
>
> > On Nov 22, 2016, at 12:47 PM, Tyler Kenney <tjkenney at us.ibm.com> wrote:
> >
> >
> > Vedant,
> >
> > I attached an updated .diff with a filecheck regression test. Look
good?
>
> Hm, I don't see the diff anywhere. Maybe it got scrubbed?
>
>
> >
> > I am far from a TableGen expert so I don't think I am the one to clean
up emitEnums(), just had the bug fix and wanted to share it.
>
> Fair enough :).
>
> vedant
>
>
> >
> > Tyler
> >
> >
> > ----- Original message -----
> > From: Vedant Kumar <vsk at apple.com>
> > To: Tyler Kenney/Marlborough/IBM at IBMUS
> > Cc: llvm-commits at lists.llvm.org
> > Subject: Re: [PATCH] TableGen InstrMapping Bug fix
> > Date: Mon, Nov 7, 2016 6:40 PM
> >
> > Your fix looks correct but should really have a test.
> >
> > We don't have any existing tests which run -gen-instr-info, but it
should be
> > possible. We'd just need to hit the emitEnums path with an entry that
sets
> > FieldValues to {a, a, a}.
> >
> > Side note: emitEnums() really needs to be cleaned up. It looks like it
has
> > cubic runtime in the worst case, and it does what looks like a totally
> > unnecessary copy to initialize FieldValues.
> >
> > best,
> > vedant
> >
> > > On Nov 2, 2016, at 7:55 AM, Tyler Kenney via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
> > >
> > >
> > >
> > > I believe I've found a bug in CodeGenMapTable.cpp.emitEnums(). I'm
developing an out-of-tree backend and I was getting an error on a tablegen
header for a duplicate enum value. emitEnums() was not properly deleting
duplicate entries, but does with the attached the patch. Apparently none of
the in-tree targets have enough InstrMapping's to trigger this bug, but my
backend defines many.
> > >
> > > I don't think there is a way to provide a test case for this without
providing a whole new backend, so hopefully we can agree that it's not
necessary. I've confirmed that all default targets still build with this
patch applied.
> > >
> > > Tyler
> > >
> > >
> > >
> > > <CodeGenMapTable.diff>_______________________________________________
> > > llvm-commits mailing list
> > > llvm-commits at lists.llvm.org
> > > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> >
> >
> >
>
>
>
> <TableGenInstrMappingDuplicateBugFix.diff>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161201/81a06c51/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: graycol.gif
Type: image/gif
Size: 105 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161201/81a06c51/attachment.gif>