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

[Jeremy Morse via Phabricator via llvm-commits]

jmorse added a comment.

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).

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.



================
Comment at: llvm/lib/Bitcode/Reader/MetadataLoader.cpp:1260
     // - Signed integer (version 0)
     // - Metadata node  (version 1)
     switch (Record[0] >> 1) {
----------------
This comment will want updating for version two.


================
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);
----------------
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).


================
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);
----------------
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.


================
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() !=
----------------
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..").


================
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))
+
----------------
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.


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

https://reviews.llvm.org/D80197