llvm dev - Jul 2014 - [LLVMdev] Fwd: Re: [PATCH] [TABLEGEN] Do not crash on intrinsics with names longer than 40 characters

Hi Manuel,

Here's another commit authored through the web interface where no 
discussion or reviewership information is apparent on the mailing list.

All we see in cases like this are a few unthreaded list posts by the 
original author followed by an SVN revision number:

http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140714/226166.html

For any patch that's submitted for review, especially in a core module 
like tablegen, I think it's worth putting the review discussion in a 
public place so we can keep track.






-------- Original Message --------
Subject: 	Re: [PATCH] [TABLEGEN] Do not crash on intrinsics with names 
longer than 40 characters
Date: 	Thu, 17 Jul 2014 11:32:18 +0000
From: 	Justin Holewinski <justin.holewinski at gmail.com>
Reply-To: 	reviews+D4537+public+fddcf5969927efcd at reviews.llvm.org
To: 	justin.holewinski at gmail.com
CC: 	llvm-commits at cs.uiuc.edu



Closed by commit rL213253 (authored by @jholewinski).

REPOSITORY
   rL LLVM

http://reviews.llvm.org/D4537

Files:
   llvm/trunk/test/TableGen/intrinsic-long-name.td
   llvm/trunk/utils/TableGen/IntrinsicEmitter.cpp

Index: llvm/trunk/utils/TableGen/IntrinsicEmitter.cpp
==================================================================---
llvm/trunk/utils/TableGen/IntrinsicEmitter.cpp
+++ llvm/trunk/utils/TableGen/IntrinsicEmitter.cpp
@@ -129,8 +129,9 @@
    for (unsigned i = 0, e = Ints.size(); i != e; ++i) {
      OS << "    " << Ints[i].EnumName;
      OS << ((i != e-1) ? ", " : "  ");
-    OS << std::string(40-Ints[i].EnumName.size(), ' ')
-      << "// " << Ints[i].Name << "\n";
+    if (Ints[i].EnumName.size() < 40)
+      OS << std::string(40-Ints[i].EnumName.size(), ' ');
+    OS << " // " << Ints[i].Name << "\n";
    }
    OS << "#endif\n\n";
  }
Index: llvm/trunk/test/TableGen/intrinsic-long-name.td
==================================================================---
llvm/trunk/test/TableGen/intrinsic-long-name.td
+++ llvm/trunk/test/TableGen/intrinsic-long-name.td
@@ -0,0 +1,32 @@
+// RUN: llvm-tblgen -gen-intrinsic %s | FileCheck %s
+// XFAIL: vg_leak
+
+class IntrinsicProperty;
+
+class ValueType<int size, int value> {
+  string Namespace = "MVT";
+  int Size = size;
+  int Value = value;
+}
+
+class LLVMType<ValueType vt> {
+  ValueType VT = vt;
+}
+
+class Intrinsic<string name, list<LLVMType> param_types = []> {
+  string LLVMName = name;
+  bit isTarget = 0;
+  string TargetPrefix = "";
+  list<LLVMType> RetTypes = [];
+  list<LLVMType> ParamTypes = param_types;
+  list<IntrinsicProperty> Properties = [];
+}
+
+def iAny : ValueType<0, 254>;
+def llvm_anyint_ty : LLVMType<iAny>;
+
+// Make sure we generate the long name without crashing
+// CHECK: this_is_a_really_long_intrinsic_name_but_we_should_still_not_crash  
// llvm.this.is.a.really.long.intrinsic.name.but.we.should.still.not.crash
+def int_foo : Intrinsic<"llvm.foo", [llvm_anyint_ty]>;
+def int_this_is_a_really_long_intrinsic_name_but_we_should_still_not_crash :
Intrinsic<"llvm.this.is.a.really.long.intrinsic.name.but.we.should.still.not.crash",
[llvm_anyint_ty]>;
+



-------------- next part --------------
A non-text attachment was scrubbed...
Name: D4537.11567.patch
Type: text/x-patch
Size: 1960 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20140717/4d1f97aa/attachment.bin>
-------------- next part --------------
_______________________________________________
llvm-commits mailing list
llvm-commits at cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

Hi Alp,

On Thu, Jul 17, 2014 at 6:25 AM, Alp Toker <alp at nuanti.com>
wrote:
> Hi Manuel,
>
> Here's another commit authored through the web interface where no
discussion
> or reviewership information is apparent on the mailing list.
If you look at the phab review, it's the same there. The only changes
on the review actually went to the mailing list.

-eric
>
> All we see in cases like this are a few unthreaded list posts by the
> original author followed by an SVN revision number:
>
>
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140714/226166.html
>
> For any patch that's submitted for review, especially in a core module
like
> tablegen, I think it's worth putting the review discussion in a public
place
> so we can keep track.
>
>
>
>
>
>
>
> -------- Original Message --------
> Subject:        Re: [PATCH] [TABLEGEN] Do not crash on intrinsics with
names
> longer than 40 characters
> Date:   Thu, 17 Jul 2014 11:32:18 +0000
> From:   Justin Holewinski <justin.holewinski at gmail.com>
> Reply-To:       reviews+D4537+public+fddcf5969927efcd at reviews.llvm.org
> To:     justin.holewinski at gmail.com
> CC:     llvm-commits at cs.uiuc.edu
>
>
>
> Closed by commit rL213253 (authored by @jholewinski).
>
> REPOSITORY
>   rL LLVM
>
> http://reviews.llvm.org/D4537
>
> Files:
>   llvm/trunk/test/TableGen/intrinsic-long-name.td
>   llvm/trunk/utils/TableGen/IntrinsicEmitter.cpp
>
> Index: llvm/trunk/utils/TableGen/IntrinsicEmitter.cpp
> ==================================================================> ---
llvm/trunk/utils/TableGen/IntrinsicEmitter.cpp
> +++ llvm/trunk/utils/TableGen/IntrinsicEmitter.cpp
> @@ -129,8 +129,9 @@
>    for (unsigned i = 0, e = Ints.size(); i != e; ++i) {
>      OS << "    " << Ints[i].EnumName;
>      OS << ((i != e-1) ? ", " : "  ");
> -    OS << std::string(40-Ints[i].EnumName.size(), ' ')
> -      << "// " << Ints[i].Name <<
"\n";
> +    if (Ints[i].EnumName.size() < 40)
> +      OS << std::string(40-Ints[i].EnumName.size(), ' ');
> +    OS << " // " << Ints[i].Name <<
"\n";
>    }
>    OS << "#endif\n\n";
>  }
> Index: llvm/trunk/test/TableGen/intrinsic-long-name.td
> ==================================================================> ---
llvm/trunk/test/TableGen/intrinsic-long-name.td
> +++ llvm/trunk/test/TableGen/intrinsic-long-name.td
> @@ -0,0 +1,32 @@
> +// RUN: llvm-tblgen -gen-intrinsic %s | FileCheck %s
> +// XFAIL: vg_leak
> +
> +class IntrinsicProperty;
> +
> +class ValueType<int size, int value> {
> +  string Namespace = "MVT";
> +  int Size = size;
> +  int Value = value;
> +}
> +
> +class LLVMType<ValueType vt> {
> +  ValueType VT = vt;
> +}
> +
> +class Intrinsic<string name, list<LLVMType> param_types = []>
{
> +  string LLVMName = name;
> +  bit isTarget = 0;
> +  string TargetPrefix = "";
> +  list<LLVMType> RetTypes = [];
> +  list<LLVMType> ParamTypes = param_types;
> +  list<IntrinsicProperty> Properties = [];
> +}
> +
> +def iAny : ValueType<0, 254>;
> +def llvm_anyint_ty : LLVMType<iAny>;
> +
> +// Make sure we generate the long name without crashing
> +// CHECK:
> this_is_a_really_long_intrinsic_name_but_we_should_still_not_crash   //
> llvm.this.is.a.really.long.intrinsic.name.but.we.should.still.not.crash
> +def int_foo : Intrinsic<"llvm.foo", [llvm_anyint_ty]>;
> +def int_this_is_a_really_long_intrinsic_name_but_we_should_still_not_crash
> :
>
Intrinsic<"llvm.this.is.a.really.long.intrinsic.name.but.we.should.still.not.crash",
> [llvm_anyint_ty]>;
> +
>
>
>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>

On 17/07/2014 20:27, Eric Christopher wrote:
> Hi Alp,
>
> On Thu, Jul 17, 2014 at 6:25 AM, Alp Toker <alp at nuanti.com> wrote:
>> Hi Manuel,
>>
>> Here's another commit authored through the web interface where no
discussion
>> or reviewership information is apparent on the mailing list.
> If you look at the phab review, it's the same there.
Does that mean no review comments were posted in the first place, or did 
they go missing?
> The only changes
> on the review actually went to the mailing list.
Sure, Manuel asked to forward incidents involving the web review system 
instead of directly checking in the individual contributor as we were 
doing before.

>
> -eric
>
>> All we see in cases like this are a few unthreaded list posts by the
>> original author followed by an SVN revision number:
>>
>>
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20140714/226166.html
>>
>> For any patch that's submitted for review, especially in a core
module like
>> tablegen, I think it's worth putting the review discussion in a
public place
>> so we can keep track.
>>
>>
>>
>>
>>
>>
>>
>> -------- Original Message --------
>> Subject:        Re: [PATCH] [TABLEGEN] Do not crash on intrinsics with
names
>> longer than 40 characters
>> Date:   Thu, 17 Jul 2014 11:32:18 +0000
>> From:   Justin Holewinski <justin.holewinski at gmail.com>
>> Reply-To:       reviews+D4537+public+fddcf5969927efcd at
reviews.llvm.org
>> To:     justin.holewinski at gmail.com
>> CC:     llvm-commits at cs.uiuc.edu
>>
>>
>>
>> Closed by commit rL213253 (authored by @jholewinski).
>>
>> REPOSITORY
>>    rL LLVM
>>
>> http://reviews.llvm.org/D4537
>>
>> Files:
>>    llvm/trunk/test/TableGen/intrinsic-long-name.td
>>    llvm/trunk/utils/TableGen/IntrinsicEmitter.cpp
>>
>> Index: llvm/trunk/utils/TableGen/IntrinsicEmitter.cpp
>>
==================================================================>> ---
llvm/trunk/utils/TableGen/IntrinsicEmitter.cpp
>> +++ llvm/trunk/utils/TableGen/IntrinsicEmitter.cpp
>> @@ -129,8 +129,9 @@
>>     for (unsigned i = 0, e = Ints.size(); i != e; ++i) {
>>       OS << "    " << Ints[i].EnumName;
>>       OS << ((i != e-1) ? ", " : "  ");
>> -    OS << std::string(40-Ints[i].EnumName.size(), ' ')
>> -      << "// " << Ints[i].Name <<
"\n";
>> +    if (Ints[i].EnumName.size() < 40)
>> +      OS << std::string(40-Ints[i].EnumName.size(), '
');
>> +    OS << " // " << Ints[i].Name <<
"\n";
>>     }
>>     OS << "#endif\n\n";
>>   }
>> Index: llvm/trunk/test/TableGen/intrinsic-long-name.td
>>
==================================================================>> ---
llvm/trunk/test/TableGen/intrinsic-long-name.td
>> +++ llvm/trunk/test/TableGen/intrinsic-long-name.td
>> @@ -0,0 +1,32 @@
>> +// RUN: llvm-tblgen -gen-intrinsic %s | FileCheck %s
>> +// XFAIL: vg_leak
>> +
>> +class IntrinsicProperty;
>> +
>> +class ValueType<int size, int value> {
>> +  string Namespace = "MVT";
>> +  int Size = size;
>> +  int Value = value;
>> +}
>> +
>> +class LLVMType<ValueType vt> {
>> +  ValueType VT = vt;
>> +}
>> +
>> +class Intrinsic<string name, list<LLVMType> param_types =
[]> {
>> +  string LLVMName = name;
>> +  bit isTarget = 0;
>> +  string TargetPrefix = "";
>> +  list<LLVMType> RetTypes = [];
>> +  list<LLVMType> ParamTypes = param_types;
>> +  list<IntrinsicProperty> Properties = [];
>> +}
>> +
>> +def iAny : ValueType<0, 254>;
>> +def llvm_anyint_ty : LLVMType<iAny>;
>> +
>> +// Make sure we generate the long name without crashing
>> +// CHECK:
>> this_is_a_really_long_intrinsic_name_but_we_should_still_not_crash   //
>> llvm.this.is.a.really.long.intrinsic.name.but.we.should.still.not.crash
>> +def int_foo : Intrinsic<"llvm.foo", [llvm_anyint_ty]>;
>> +def
int_this_is_a_really_long_intrinsic_name_but_we_should_still_not_crash
>> :
>>
Intrinsic<"llvm.this.is.a.really.long.intrinsic.name.but.we.should.still.not.crash",
>> [llvm_anyint_ty]>;
>> +
>>
>>
>>
>>
>> _______________________________________________
>> LLVM Developers mailing list
>> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>>
-- 
http://www.nuanti.com
the browser experts