[PATCH] TableGen InstrMapping Bug fix

Thu Dec 1 11:49:26 PST 2016

Committed in r288408.

thanks,
vedant

> On Dec 1, 2016, at 5:38 AM, Tyler Kenney <tjkenney at us.ibm.com> wrote:
> 
> 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
> 
> 
> <graycol.gif>Vedant Kumar ---11/30/2016 06:27:36 PM---I'm sorry for the delay. This LGTM. Let me know if you'd like me to commit this for you.
> 
> 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>
> 
> 
>