hans added a comment.

I don't really know about the functionality here, just adding a few comments on 
the code itself.



================
Comment at: clang/lib/CodeGen/CGDebugInfo.cpp:1966
+  QualType PointeeTy = D.getTypePtr()->getPointeeType();
+  llvm::DIType *DI = getOrCreateType(PointeeTy, getOrCreateFile(Loc));
+  CI->setMetadata("heapallocsite", DI);
----------------
I don't really know this code, but does this also work for void pointers, i.e. 
the if statement in the old code was unnecessary?


================
Comment at: clang/test/CodeGen/debug-info-codeview-heapallocsite.c:19
+// FIXME: Currently not testing that call_alloc_void has metadata because the
+// metadata is not set when the type is void.
+
----------------
Is it not set with your new code though?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp:1078
+      MCSymbol *EndLabel = std::get<1>(HeapAllocSite);
+      DIType *DITy = cast<DIType>(std::get<2>(HeapAllocSite));
+
----------------
Is the cast necessary? Couldn't the tuple member be made a DIType* in the first 
place?


================
Comment at: llvm/test/CodeGen/X86/label-heapallocsite.ll:1
+; RUN: llc -O0 < %s | FileCheck %s
+; FIXME: Add test for llc with optimizations once it is implemented.
----------------
Does llc have a "-fast-isel" flag or similar that could be used instead, to 
make it more clear that it's fast-isel that's significant for the test?


================
Comment at: llvm/test/CodeGen/X86/label-heapallocsite.ll:16
+; ModuleID = 'heapallocsite.c'
+source_filename = "heapallocsite.c"
+target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
----------------
I guess the ModuleID and source_filename are unnecessary, so I'd dro pthem.


================
Comment at: llvm/test/CodeGen/X86/label-heapallocsite.ll:51
+attributes #0 = { noinline nounwind optnone 
"correctly-rounded-divide-sqrt-fp-math"="false" "disable-tail-calls"="false" 
"less-precise-fpmad"="false" "min-legal-vector-width"="0" 
"no-frame-pointer-elim"="false" "no-infs-fp-math"="false" 
"no-jump-tables"="false" "no-nans-fp-math"="false" 
"no-signed-zeros-fp-math"="false" "no-trapping-math"="false" 
"stack-protector-buffer-size"="8" "target-features"="+cx8,+mmx,+sse,+sse2,+x87" 
"unsafe-fp-math"="false" "use-soft-float"="false" }
+attributes #1 = { "correctly-rounded-divide-sqrt-fp-math"="false" 
"disable-tail-calls"="false" "less-precise-fpmad"="false" 
"no-frame-pointer-elim"="false" "no-infs-fp-math"="false" 
"no-nans-fp-math"="false" "no-signed-zeros-fp-math"="false" 
"no-trapping-math"="false" "stack-protector-buffer-size"="8" 
"target-features"="+cx8,+mmx,+sse,+sse2,+x87" "unsafe-fp-math"="false" 
"use-soft-float"="false" }
+
----------------
Both sets of attributes seem unnecessary so could probably be removed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60800



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to