[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#2051742 <https://reviews.llvm.org/D80197#2051742>, @aprantl wrote:

> I think this is pretty good. A couple of minor comments inline.


Thanks a lot for your comments. I shall update the patch in next version.



================
Comment at: llvm/include/llvm/IR/DIBuilder.h:577
+    DISubrange *getOrCreateSubrange(Metadata *Count, Metadata *LowerBound,
+                                    Metadata *UpperBound, Metadata *Stride);
 
----------------
aprantl wrote:
> Why does this take a `Metadata *` over a `DIExpression *` or something else more specific?
> Update: The PointerUnion<> that is BoundType?
Yes, that is an option. PointerUnion and Metadata both can represent signed int (ConstantAsMetadata), DIVariable and DIExpression. PointerUnion is more specific as you mentioned, as other places will reject what extra types Metadata can have. I just followed the way Count is handled which also has PointerUnion type.  


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:355
+    if (!getRawLowerBound())
+      return BoundType();
+
----------------
aprantl wrote:
> I think it might be reasonable to assert() that the type is well-formed here.
Sure, I shall add that in next version of patch.


================
Comment at: llvm/include/llvm/IR/DebugInfoMetadata.h:364
+    if (auto *MD = dyn_cast<DIExpression>(getRawLowerBound()))
+      return BoundType(MD);
+
----------------
aprantl wrote:
> Can you move these function bodies into the .cpp file?
Sure. It will be updated in next version of patch.


================
Comment at: llvm/lib/IR/Verifier.cpp:913
+  AssertDI(!N.getRawStride() || N.getStride(),
+           "Stride must be signed constant or DIVariable or DIExpression", &N);
 }
----------------
aprantl wrote:
> In other DINodes we have getSomeAttribute() method assert when it's malformed, but the getRawAttribute() returns a raw Metadata pointer, and the Verifier checks that the Metadata node is a legal one.
Thanks for your comment. It will be included in next version of patch.


================
Comment at: llvm/unittests/IR/MetadataTest.cpp:1269
+
+  EXPECT_EQ(N, DISubrange::get(Context, nullptr, LE, UE, SE));
+}
----------------
aprantl wrote:
> Let me know if I missed it: Is there a test for the LLVMContextImpl hashing here? Where it's tested that two DISubranges that differ in only one attribute compare differently?
Thanks for pointing this out. It will be included in next version of patch.


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

https://reviews.llvm.org/D80197