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

[Alok Kumar Sharma via Phabricator via llvm-commits]

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

In D80197#2048855 <https://reviews.llvm.org/D80197#2048855>, @jmorse wrote:

> I'm not incredibly familiar with bitcode and metadata, or Fortran at all, but left some comments which hopefully help. This in general looks pretty good -- do you think it's worth making Count and UpperBound mutually exclusive? Otherwise there's the chance to represent the same information twice (and thus inconsistently).


Thanks for all your valuable comments. Yes, I shall update the patch for this.

> There's a reasonable amount of code (DWARF printing for example) that's repeated, IMO it would be worth using lambdas more often to ease readability.

Thanks for your suggestion. I shall incorporate this in next version.



================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:1260
     // - Signed integer (version 0)
     // - Metadata node  (version 1)
     switch (Record[0] >> 1) {
----------------
jmorse wrote:
> This comment will want updating for version two.
Thanks for pointing this out. It'll be updated in next version.


================
Comment at: llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:1530
                                           unsigned Abbrev) {
-  const uint64_t Version = 1 << 1;
+  const uint64_t Version = 1 << 2;
   Record.push_back((uint64_t)N->isDistinct() | Version);
----------------
jmorse wrote:
> Could I suggest `2 << 1` instead? From my very limited understanding, the Distinct field goes in the bottom bit, and writing this as `2 << 1` makes it clear "we're shifting the version up one bit", wheras `1 << 2` makes it feel like the number is a flag.
> 
> (This may be a style thing, and thus unimportant).
Yes, true. Makes sense. It'll be updated in next version.


================
Comment at: llvm/lib/IR/AsmWriter.cpp:1870-1875
+  else if (auto *LV = N->getLowerBound().dyn_cast<DIVariable *>())
+    Printer.printMetadata("lowerBound", LV, /*ShouldSkipNull */ true);
+  else
+    Printer.printMetadata("lowerBound",
+                          N->getLowerBound().dyn_cast<DIExpression *>(),
+                          /*ShouldSkipNull */ true);
----------------
jmorse wrote:
> As far as I can tell, printMetadata isn't overloaded, so you should be able to pass the non-null un-casted result of getLowerBound to printMetadata, without having to examine its type further.
Thanks for pointing this out. It'll be updated in next version.


================
Comment at: llvm/lib/IR/LLVMContextImpl.h:364-365
+
+    if (auto *RHSStride = RHS->getStride().dyn_cast<ConstantInt *>()) {
+      if (auto *MD = dyn_cast<ConstantAsMetadata>(Stride))
+        if (RHSStride->getSExtValue() !=
----------------
jmorse wrote:
> Unless I'm reading this wrong, when:
>  * RHSStrideis a ConstantInt,
>  * Strideis something else
> The first `if` will be true, the second `if` false, and we won't `return false` here despite the two things being different.
> 
> That's probably fixed by combining the first two ifs into one larger precondition ("if both are CosntantInts..").
Yes, correct. It'll be updated in next version.


================
Comment at: llvm/test/Bitcode/fortranSubrange.ll:6
+;; Test whether bounds are generated correctly.
+; CHECK: !{{[0-9]+}} = !DISubrange(lowerBound: 3, upperBound: !{{[0-9]+}}, stride: !DIExpression(DW_OP_constu, 4))
+
----------------
jmorse wrote:
> In here (and other tests) you probably want to capture the upperBound metadata node number and check that it's referring to the correct node too. !4 would be a legitimate node here, but as empty metadata, wouldn't be correct.
Yes, thanks for your comment. It'll be updated in next version.


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

https://reviews.llvm.org/D80197