[PATCH] TableGen InstrMapping Bug fix

[Tyler Kenney via llvm-commits]

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)



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


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161123/0347fcd4/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/20161123/0347fcd4/attachment.gif>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: TableGenInstrMappingDuplicateBugFix.diff
Type: application/octet-stream
Size: 2607 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20161123/0347fcd4/attachment.obj>