[PATCH] D79190: llvm rejects DWARF operator DW_OP_lit[1-31] in IR

[Orlando Cazalet-Hyams via Phabricator via llvm-commits]

Orlando added a comment.

In D79190#2018830 <https://reviews.llvm.org/D79190#2018830>, @dblaikie wrote:

> In D79190#2018046 <https://reviews.llvm.org/D79190#2018046>, @aprantl wrote:
>
> > In D79190#2016252 <https://reviews.llvm.org/D79190#2016252>, @alok wrote:
> >
> > > In D79190#2015441 <https://reviews.llvm.org/D79190#2015441>, @aprantl wrote:
> > >
> > > > > when this stuff was added, do you recall if there was any particular goal around that? Was lit0 added for some specific reason/separate from adding litN? Or was it just an oversight? And/or do you have thoughts on which way this should go today?
> > > >
> > > > As I said yesterday, I think it would be better to aim for a canonical representation in LLVM to make transformations easier. A quick grep for `DW_OP_lit0` in `llvm/lit` shows no current use of this, so we could remove it in favor of DW_OP_constu 0.
> > > >  Doing some archeology, lit0 was added by @vsk in https://reviews.llvm.org/D48676 but that code must have been removed since. We could easily write a bitcode upgrade to canonicalize existing occurrences of lit0.
> > >
> > >
> > > Should we do upgrade to other lits (1-31) also in place of rejecting them ?
> >
> >
> > I'm assuming that once we have the code for upgrading lit0, handling the other lit operators will not really add any complexity, so I'm fine either way, if that makes things easier.
>
>
> I'd be a little against it - adding compatibility for something we were never compatible with in the first place - but not strongly enough to make/break it.


+1 a little against it - I don't see the appeal of moving the DIExpressions any closer to the final (DWARF) output. It's more on principle than against this specific change though; As @aprantl pointed out this case will be trivial to handle. But on the other hand, the lit0 case which already exists shows that setting a precedent could be significant, IMO.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79190/new/

https://reviews.llvm.org/D79190