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

If this information is useful, please help other people find it:
Share via:

Alp Toker

2014-Jul-17 17:43 UTC

[LLVMdev] Fwd: Re: [PATCH] [TABLEGEN] Do not crash on intrinsics with names longer than 40 characters

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

Eric Christopher

2014-Jul-17 17:44 UTC

head link

[LLVMdev] Fwd: Re: [PATCH] [TABLEGEN] Do not crash on intrinsics with names longer than 40 characters

On Thu, Jul 17, 2014 at 10:43 AM, Alp Toker <alp at nuanti.com>
wrote:>
> 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?
Unknown unknowns. There are no comments on the review in phab.

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

Justin Holewinski

2014-Jul-17 18:06 UTC

head link

[LLVMdev] Fwd: Re: [PATCH] [TABLEGEN] Do not crash on intrinsics with names longer than 40 characters

On Thu, 2014-07-17 at 10:44 -0700, Eric Christopher
wrote:> On Thu, Jul 17, 2014 at 10:43 AM, Alp Toker <alp at nuanti.com>
wrote:
> >
> > 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?
> 
> Unknown unknowns. There are no comments on the review in phab.
Since it was a minor change not affecting functionality, just fixing a
crash, I left the review open for a day for objections and then
committed.
> 
> -eric
> 
> >
> >
> >> 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
> >
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev

-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may
contain
confidential information.  Any unauthorized review, use, disclosure or
distribution
is prohibited.  If you are not the intended recipient, please contact the sender
by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

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

[LLVMdev] Fwd: Re: [PATCH] [TABLEGEN] Do not crash on intrinsics with names longer than 40 characters

[LLVMdev] Fwd: Re: [PATCH] [TABLEGEN] Do not crash on intrinsics with names longer than 40 characters

[LLVMdev] Fwd: Re: [PATCH] [TABLEGEN] Do not crash on intrinsics with names longer than 40 characters