llvm dev - Feb 2020 - [LLVM][DISubprogram][LL format updation query] Question regarding moving DISubprogram DIFlags to DISPFlag.

Yes, removing the support for isLocal, isDefinition fields completely from ll
files, currently the LLParser still parses it. I want to remove it and update
the all the ll files which still uses it.
Also the metadata read will support old format, no changes in that.

so if ll file has isLocal and isDefinition it will result in parser error. But
the bitcode read will work as usual.

- Chirag.

-----Original Message-----
From: Djordje Todorovic <djordje.todorovic at rt-rk.com> 
Sent: 20 February 2020 14:16
To: Chirag Patel <Chirag at raincode.com>; llvm-dev at lists.llvm.org
Subject: Re: [llvm-dev] [LLVM][DISubprogram][LL format updation query] Question
regarding moving DISubprogram DIFlags to DISPFlag.

Hi Chirag,

On 20.2.20. 07:51, Chirag Patel via llvm-dev wrote:
> Hello,
> 
>  
> 
> In regard to the review request https://reviews.llvm.org/D74470,
> 
> I am trying to move five of the DIFlags to DISPFlag for the moment namely
DIFlagExplicit, DIFlagPrototyped, DIFlagNoReturn, DIFlagThunk,
DIFlagAllCallsDescribed.
> 
> The llvm ir format for DISubprogram currently has backword compatibility
where the isLocal, isDefinition, virtuality, isOptimized and SPFlags are
mutually exclusive.
> 
> My question is,
> 
> is it a good idea to remove the booleans support'(isLocal,
isDefinition) and move most of it to spflags and flags in llvm ir?
But it was already "done", we currently have the
'DISPFlagLocalToUnit' and 'DISPFlagDefinition' (please take a
look into the https://reviews.llvm.org/D54755 and
https://reviews.llvm.org/D59288).

The llvm ir backward compatibility does not list the clear requirements on
documentations page. This change affects more then 750 ll
files.
> 
I am not sure what change will take 750 ll files? Removing the 'isLocal'
and 'isDefinition'? I think the role of LLVM IR backward compatibility
is to support interpretation of the old metadata in terms of the newest one.
Therefore, if there is an 'isDefinition' metadata field, that should be
interpreted as 'DISPFlagDefinition'.

> 
> Regards,
> 
> Chirag Partel.
>

Hi Chirag,

On 20.2.20. 09:57, Chirag Patel wrote:
> Yes, removing the support for isLocal, isDefinition fields completely from
ll files, currently the LLParser still parses it. I want to remove it and update
the all the ll files which still uses it.
Could you please describe what is the benefit of that?
> Also the metadata read will support old format, no changes in that.
>
> so if ll file has isLocal and isDefinition it will result in parser error.
But the bitcode read will work as usual.
>
AFAIK, there are no compatibility guarantees for textual LLVM IR, so we only
need to support the bitcode auto-upgrade. So, that would be acceptable if there
is motivation for doing that.
In addition, I'm not sure if this was documented somewhere, so we could
improve the documentation for the metadata backward compatibility.
 
> - Chirag.
> 
> -----Original Message-----
> From: Djordje Todorovic <djordje.todorovic at rt-rk.com> 
> Sent: 20 February 2020 14:16
> To: Chirag Patel <Chirag at raincode.com>; llvm-dev at lists.llvm.org
> Subject: Re: [llvm-dev] [LLVM][DISubprogram][LL format updation query]
Question regarding moving DISubprogram DIFlags to DISPFlag.
> 
> Hi Chirag,
> 
> On 20.2.20. 07:51, Chirag Patel via llvm-dev wrote:
>> Hello,
>>
>>  
>>
>> In regard to the review request https://reviews.llvm.org/D74470,
>>
>> I am trying to move five of the DIFlags to DISPFlag for the moment
namely DIFlagExplicit, DIFlagPrototyped, DIFlagNoReturn, DIFlagThunk,
DIFlagAllCallsDescribed.
>>
>> The llvm ir format for DISubprogram currently has backword
compatibility where the isLocal, isDefinition, virtuality, isOptimized and
SPFlags are mutually exclusive.
>>
>> My question is,
>>
>> is it a good idea to remove the booleans support'(isLocal,
isDefinition) and move most of it to spflags and flags in llvm ir?
> 
> But it was already "done", we currently have the
'DISPFlagLocalToUnit' and 'DISPFlagDefinition' (please take a
look into the https://reviews.llvm.org/D54755 and
https://reviews.llvm.org/D59288).
> 
> The llvm ir backward compatibility does not list the clear requirements on
documentations page. This change affects more then 750 ll files.
>>
> 
> I am not sure what change will take 750 ll files? Removing the
'isLocal' and 'isDefinition'? I think the role of LLVM IR
backward compatibility is to support interpretation of the old metadata in terms
of the newest one. Therefore, if there is an 'isDefinition' metadata
field, that should be interpreted as 'DISPFlagDefinition'.
> 
> 
>>
>> Regards,
>>
>> Chirag Partel.
>>
Best, 
Djordje

> Could you please describe what is the benefit of that?
Currently there are two ways to provide DISPFlagDefinition, via bool and SPFlag,
I would like to make it only via SPFlags, it will be NFC and it will make the
changes in parser simpler for moving five flags from from DIFlags to DISPFlags.
Currently parser checks the presence of SPFlags to see if the definition is
present in bool or spflag if I move flags to spflags, it will create problems
hence some of the flags may be present in spflags and in Boolean, as in example
spFlags: DISPFlagThunk, isLocal: True.

- Chirag.


-----Original Message-----
From: Djordje Todorovic <djordje.todorovic at rt-rk.com> 
Sent: 20 February 2020 14:40
To: Chirag Patel <Chirag at raincode.com>; llvm-dev at lists.llvm.org
Subject: Re: [llvm-dev] [LLVM][DISubprogram][LL format updation query] Question
regarding moving DISubprogram DIFlags to DISPFlag.

Hi Chirag,

On 20.2.20. 09:57, Chirag Patel wrote:
> Yes, removing the support for isLocal, isDefinition fields completely from
ll files, currently the LLParser still parses it. I want to remove it and update
the all the ll files which still uses it.
Could you please describe what is the benefit of that?
> Also the metadata read will support old format, no changes in that.
>
> so if ll file has isLocal and isDefinition it will result in parser error.
But the bitcode read will work as usual.
>
AFAIK, there are no compatibility guarantees for textual LLVM IR, so we only
need to support the bitcode auto-upgrade. So, that would be acceptable if there
is motivation for doing that.
In addition, I'm not sure if this was documented somewhere, so we could
improve the documentation for the metadata backward compatibility.
 
> - Chirag.
> 
> -----Original Message-----
> From: Djordje Todorovic <djordje.todorovic at rt-rk.com> 
> Sent: 20 February 2020 14:16
> To: Chirag Patel <Chirag at raincode.com>; llvm-dev at lists.llvm.org
> Subject: Re: [llvm-dev] [LLVM][DISubprogram][LL format updation query]
Question regarding moving DISubprogram DIFlags to DISPFlag.
> 
> Hi Chirag,
> 
> On 20.2.20. 07:51, Chirag Patel via llvm-dev wrote:
>> Hello,
>>
>>  
>>
>> In regard to the review request https://reviews.llvm.org/D74470,
>>
>> I am trying to move five of the DIFlags to DISPFlag for the moment
namely DIFlagExplicit, DIFlagPrototyped, DIFlagNoReturn, DIFlagThunk,
DIFlagAllCallsDescribed.
>>
>> The llvm ir format for DISubprogram currently has backword
compatibility where the isLocal, isDefinition, virtuality, isOptimized and
SPFlags are mutually exclusive.
>>
>> My question is,
>>
>> is it a good idea to remove the booleans support'(isLocal,
isDefinition) and move most of it to spflags and flags in llvm ir?
> 
> But it was already "done", we currently have the
'DISPFlagLocalToUnit' and 'DISPFlagDefinition' (please take a
look into the https://reviews.llvm.org/D54755 and
https://reviews.llvm.org/D59288).
> 
> The llvm ir backward compatibility does not list the clear requirements on
documentations page. This change affects more then 750 ll files.
>>
> 
> I am not sure what change will take 750 ll files? Removing the
'isLocal' and 'isDefinition'? I think the role of LLVM IR
backward compatibility is to support interpretation of the old metadata in terms
of the newest one. Therefore, if there is an 'isDefinition' metadata
field, that should be interpreted as 'DISPFlagDefinition'.
> 
> 
>>
>> Regards,
>>
>> Chirag Partel.
>>
Best, 
Djordje

> On Feb 20, 2020, at 1:10 AM, Djordje Todorovic via llvm-dev <llvm-dev at
lists.llvm.org> wrote:
> 
> AFAIK, there are no compatibility guarantees for textual LLVM IR, so we
only need to support the bitcode auto-upgrade. So, that would be acceptable if
there is motivation for doing that.
This is correct. We take binary bitcode upgrading seriously, and occasionally
implement textual IR assembler upgrades out of sheer laziness, not wanting to
update tons of tests. However, it is perfectly acceptable to update and cleanup
lots of tests, if the commit message contains a sed/perl/python script to
perform the update, so people can apply it on their downstream fork's
testcases. This is particularly true if the upgrade mostly affects IR *input* (I
expect this to be the case here) — we have too be more careful when updating
CHECK-lines since they often not as amenable to upgrading via sed.

-- adrian