[PATCH] D102978: [WebAssembly] Add TargetInstrInfo::getCalleeOperand

Djordje Todorovic via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Mon May 24 23:52:12 PDT 2021


djtodoro added a comment.

LGTM (with the comments addressed)



================
Comment at: llvm/lib/Target/WebAssembly/Utils/WebAssemblyUtilities.cpp:94
   case WebAssembly::RET_CALL_INDIRECT_S:
-    return MI.getOperand(MI.getNumOperands() - 1);
+    return MI.getOperand(MI.getNumExplicitOperands() - 1);
   default:
----------------
By doing this, the patch does a bit more then described in the summary. I think this is should be a separate change.


================
Comment at: llvm/test/DebugInfo/WebAssembly/call-site.ll:3
+
+; This checks if the call site informstion is correctly written in debug info.
+; This is a regression test for the bug that DwarfDebug unconditionally assumed
----------------
Super nit: The highlevel comments starts with double `;;`

typo: information 


================
Comment at: llvm/test/DebugInfo/WebAssembly/call-site.ll:36
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, producer: "clang version 11.0.0 (https://github.com/llvm/llvm-project.git ed7aaf832444411ce93aa0443425ce401f5c7a8e)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug)
+!1 = !DIFile(filename: "test.c", directory: "/home/llvm-project")
----------------
super nit: producer could be "clang version 11.0.0" only



================
Comment at: llvm/test/DebugInfo/WebAssembly/dbg-value-list.ll:36
 !5 = !{i32 1, !"wchar_size", i32 4}
-!6 = distinct !DISubprogram(name: "", scope: !1, file: !1, line: 3, type: !7, scopeLine: 3, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0)
+!6 = distinct !DISubprogram(name: "dbg_value_list_test", scope: !1, file: !1, line: 3, type: !7, scopeLine: 3, flags: DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0)
 !7 = !DISubroutineType(types: !{null})
----------------
aheejin wrote:
> This is just a drive-by name fix and unrelated to this CL
This should be a separate change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D102978



More information about the llvm-commits mailing list