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

[Alok Kumar Sharma via Phabricator via llvm-commits]

alok marked 8 inline comments as done.
alok added a comment.

In D79190#2013650 <https://reviews.llvm.org/D79190#2013650>, @probinson wrote:

> If I take your C input program and compile it to a .o file, I see that we do emit (DW_OP_lit13; DW_OP_lit0; DW_OP_mul; DW_OP_stack_value) for "var2".
>
> I think what you are saying is, we do not accept DW_OP_litN as *input* in the IR.  Is that correct?  If so, please make sure the commit log states that.


Yes correct. The C program works because clang generates "DW_OP_constu, 5", which is optimized to DW_OP_lit5, but it doesnt accept DW_OP_lit5 in IR. I missed to state that clearly in commit log. I shall update the commit log. Thanks for your review.

In D79190#2014425 <https://reviews.llvm.org/D79190#2014425>, @aprantl wrote:

> I think this may actually be to some degree intentional. In LLVM IR. What's the use-case for this? Currently in LLVM IR a constant is always represented by a DW_OP_const(u) which makes the expressions easier to parse. LLVM knows how to *emit* DW_OP_litX, but I'm not sure what we gain from accepting it in LLVM IR.


Actually I found this while working on debugability support for flang. It looked strange that DW_OP_lit0 works but not other values. I understand that the same can be represented by DW_OP_const(u) but the smaller representation with DW_OP_lit is tempting. Supporting the same from completeness point of view as well as avoiding unnecessary issue reporting by the new front-ends emitting DW_OP_litX looked tempting.



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfExpression.cpp:400
       continue;
     } else if (OpNum >= dwarf::DW_OP_breg0 && OpNum <= dwarf::DW_OP_breg31) {
       addBReg(OpNum - dwarf::DW_OP_breg0, Op->getArg(0));
----------------
probinson wrote:
> I know you did not introduce this, so I won't insist you change it, but LLVM style says do not use `else` after a `continue`.
Thanks for pointing this out, I shall correct this.


================
Comment at: llvm/test/DebugInfo/dwarfdump-litN.ll:4
+
+; RUN: %llc_dwarf %s -O2 -filetype=obj -mtriple x86_64-unknown-linux -o %t.o
+; RUN: llvm-dwarfdump  %t.o | FileCheck %s
----------------
probinson wrote:
> Remove the `-mtriple`, because you are using the %llc_dwarf substitution, which will add `-mtriple` when necessary.
Thanks for your comment. I shall incorporate this.


================
Comment at: llvm/test/DebugInfo/dwarfdump-litN.ll:14
+
+; 2. Test whetehr DW_OP_lit5 works.
+; CHECK-LABEL:       DW_TAG_variable
----------------
probinson wrote:
> Typo, "whether"
Thanks for pointing this out. I shall incorporate this.


================
Comment at: llvm/test/DebugInfo/dwarfdump-litN.ll:36
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
----------------
probinson wrote:
> Remove the target triple line, so the input will work for any target.
Thanks. I shall update this in next version.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79190