[PATCH] D80197: [DebugInfo] Upgrade DISubrange to support Fortran dynamic arrays

[Alok Kumar Sharma via Phabricator via llvm-commits]

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

In D80197#2056213 <https://reviews.llvm.org/D80197#2056213>, @aprantl wrote:

> Can you confirm that this patch doesn't cause us to emit a `DW_AT_lower_bound(0)` where we previously didn't?


Yes, it would not emit DW_AT_lower_bound(0) for C and it would also not emit DW_AT_lower_bound(1) for fortran and similarly for other language their defaults. I'll add test case for this.



================
Comment at: clang/test/CodeGen/debug-info-matrix-types.c:14
+  // CHECK: [[COLS]] = !DISubrange(count: 3, lowerBound: 0)
+  // CHECK: [[ROWS]] = !DISubrange(count: 2, lowerBound: 0)
   // CHECK: [[EXPR_A]] = !DILocalVariable(name: "a", arg: 1, {{.+}} type: [[PTR_TY]])
----------------
aprantl wrote:
> Should we just make lowerBound implicit and only print it when it is non-null?
Currently clang front-end is using old constructor for DISubrange which accepts lower as int, while using old constructor it passes zero (0) for lowerBound so it is explicit. Using new constructor and passing nullptr for lowerBound would make it not to be printed.


================
Comment at: llvm/lib/IR/AsmWriter.cpp:1866
+  // not be assumed as 0, as it depends on other factors like language, and
+  // it can also be calcualted as (upperBound - count + 1)
+  auto *LBound = N->getRawLowerBound();
----------------
aprantl wrote:
> `calculated`
> 
> I think this comment is wrong, since it doesn't account for stride.
> 
> What about: `A lowerBound of constant 0 should not be skipped, since it is different from an unspecified lower bound (= nullptr)`.
> 
> Similarly, should the existing use-cases in the test suite be nullptr values or constant zero?
Thanks for pointing it out and suggesting replacement. It'll be incorporated.


================
Comment at: llvm/unittests/IR/MetadataTest.cpp:1266
+  EXPECT_NE(N, DISubrange::get(Context, nullptr, LV, UV, SEother));
+  EXPECT_NE(N, DISubrange::get(Context, nullptr, LV, UV, SIother));
+}
----------------
aprantl wrote:
> Should we also test that LV/UV of 0 is distinct from nullptr?
Sure, it'll be added in next version of patch.


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

https://reviews.llvm.org/D80197