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? The llvm ir backward compatibility does not list the clear requirements on documentations page. This change affects more then 750 ll files.
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?
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'.
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.
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.
From: Djordje Todorovic <djordje.todorovic@rt-rk.com>
Sent: 20 February 2020 14:16
To: Chirag Patel <Chirag@raincode.com>; llvm-dev@lists.llvm.org
Subject: Re: [llvm-dev] [LLVM][DISubprogram][LL format updation query] Question regarding moving DISubprogram DIFlags to DISPFlag.
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?
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'.
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.
I see, since supporting older formats within LLParser is used only to avoid updating a bunch of tests (but not required), I do not see any obstacle to doing that (as long as we keep the bitcode backward compatibility).
If it makes the patches easier to review, one possibility is to
leave scaffolding in place to support the old textual format while
moving the actual flags around. Then in a follow-up patch, remove
the support for the old textual format and update the .ll tests.
(Of course you would need to preserve support for the old .bc format.)
I think it is reasonable to assume that there are no mixed-mode .ll
files; that is, if you see any of the old bool flags, the file is
using the old format and not the new format. So you would not need
to worry about merging old-style flags and new-style flags.
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.
f it makes the patches easier to review, one possibility is to leave scaffolding in place to support the old textual format while moving the actual flags around. Then in a follow-up patch, remove the support for the old > textual format and update the .ll tests.
(Of course you would need to preserve support for the old .bc format.)
think it is reasonable to assume that there are no mixed-mode .ll files; that is, if you see any of the old bool flags, the file is using the old format and not the new format. So you would not need to worry about merging old-style flags and new-style flags.
Thanks, I will split the patch, first move flags to spflags and then update the ll format.
))