-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[LLVM][IR] Add location tracking to LLVM IR parser #155797
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 15 commits
ecf591c
06926e9
1fdf13c
2772cd8
b05d11a
458599b
ee39ed1
b0c5318
416514e
35ca1a5
b3d8254
23dcc6b
06d5265
4e08921
3da9e9d
4b3bc0e
ed7a04a
e6142b5
f5da73c
10a2b75
17b5753
737c5e0
72b89e5
41284df
ff9a33d
008ae63
a44ef20
f201d1f
1de2447
4d51839
77385c0
f58e0ad
75e5b57
07689fc
66ce6b6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
//===-- AsmParserContext.h --------------------------------------*- C++ -*-===// | ||
// | ||
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. | ||
// See https://llvm.org/LICENSE.txt for license information. | ||
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
#ifndef LLVM_ASMPARSER_ASMPARSER_STATE_H | ||
#define LLVM_ASMPARSER_ASMPARSER_STATE_H | ||
|
||
#include "llvm/ADT/DenseMap.h" | ||
#include "llvm/IR/Value.h" | ||
#include <optional> | ||
|
||
namespace llvm { | ||
|
||
/// Registry of file location information for LLVM IR constructs | ||
Bertik23 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
/// | ||
/// This class provides access to the file location information | ||
/// for various LLVM IR constructs. Currently, it supports Function, | ||
/// BasicBlock and Instruction locations. | ||
/// | ||
/// When available, it can answer queries about what is at a given | ||
/// file location, as well as where in a file a given IR construct | ||
/// is. | ||
/// | ||
/// This information is optionally emitted by the LLParser while | ||
/// it reads LLVM textual IR. | ||
class AsmParserContext { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The name of this class makes it sound like something that should be owned by the LLParser. Maybe something like LLParserLocationInfo? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think AsmParserContext is better, as it's more future proof. In the future we might want to add some more info, other than location. As for the LLParser vs AsmParser, I think AsmParser is better since it uses what is parsed other then how it's parserd. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tend to agree with @arichardson -- I'd expect AsmParserContext to be something internally used by AsmParser. What kind of future extensions do you have in mind here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is supposed to be used similarly to AsmParserState in MLIR, In tools, that need to know some things from the parser, in this case we want to use it to be able to access locations or instructions, functions, ... in IR files for the purposes of navigating in the file via a LSP server (such as goto definition/references) |
||
DenseMap<Function *, FileLocRange> Functions; | ||
arichardson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
DenseMap<BasicBlock *, FileLocRange> Blocks; | ||
DenseMap<Instruction *, FileLocRange> Instructions; | ||
|
||
public: | ||
std::optional<FileLocRange> getFunctionLocation(const Function *) const; | ||
std::optional<FileLocRange> getBlockLocation(const BasicBlock *) const; | ||
std::optional<FileLocRange> getInstructionLocation(const Instruction *) const; | ||
/// Get the function at the requested location range. | ||
/// If no single function occupies the queried range, or the record is | ||
/// missing, a nullptr is returned. | ||
Function *getFunctionAtLocation(const FileLocRange &) const; | ||
/// Get the function at the requested location. | ||
/// If no function occupies the queried location, or the record is missing, a | ||
/// nullptr is returned. | ||
Function *getFunctionAtLocation(const FileLoc &) const; | ||
/// Get the block at the requested location range. | ||
/// If no single block occupies the queried range, or the record is missing, a | ||
/// nullptr is returned. | ||
BasicBlock *getBlockAtLocation(const FileLocRange &) const; | ||
/// Get the block at the requested location. | ||
/// If no block occupies the queried location, or the record is missing, a | ||
/// nullptr is returned. | ||
BasicBlock *getBlockAtLocation(const FileLoc &) const; | ||
/// Get the instruction at the requested location range. | ||
/// If no single instruction occupies the queried range, or the record is | ||
/// missing, a nullptr is returned. | ||
Instruction *getInstructionAtLocation(const FileLocRange &) const; | ||
/// Get the instruction at the requested location. | ||
/// If no instruction occupies the queried location, or the record is missing, | ||
/// a nullptr is returned. | ||
Instruction *getInstructionAtLocation(const FileLoc &) const; | ||
bool addFunctionLocation(Function *, const FileLocRange &); | ||
bool addBlockLocation(BasicBlock *, const FileLocRange &); | ||
bool addInstructionLocation(Instruction *, const FileLocRange &); | ||
}; | ||
} // namespace llvm | ||
|
||
#endif |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -17,18 +17,21 @@ | |||||
#include "llvm/ADT/APFloat.h" | ||||||
#include "llvm/ADT/APSInt.h" | ||||||
#include "llvm/Support/SMLoc.h" | ||||||
#include "llvm/Support/SourceMgr.h" | ||||||
#include <string> | ||||||
|
||||||
namespace llvm { | ||||||
class Type; | ||||||
class SMDiagnostic; | ||||||
class SourceMgr; | ||||||
class LLVMContext; | ||||||
|
||||||
class LLLexer { | ||||||
const char *CurPtr; | ||||||
StringRef CurBuf; | ||||||
|
||||||
// The the end (exclusive) of the current token | ||||||
|
// The the end (exclusive) of the current token | |
// The end (exclusive) of the current token. |
Also there is a mismatch here between the comment referring to "current token" and the member name being PrevTok -- which one is it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that slipped through, it is the previous token
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ///
for doc comments.
Bertik23 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -13,6 +13,7 @@ | |||||
#ifndef LLVM_ASMPARSER_LLPARSER_H | ||||||
#define LLVM_ASMPARSER_LLPARSER_H | ||||||
|
||||||
#include "AsmParserContext.h" | ||||||
|
#include "AsmParserContext.h" | |
#include "llvm/AsmParser/AsmParserContext.h" |
Also incorrect in the line below, probably this wasn't adjusted when the header was exported...
Bertik23 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,89 @@ | ||
//===-- AsmParserContext.cpp ------------------------------------*- C++ -*-===// | ||
// | ||
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. | ||
// See https://llvm.org/LICENSE.txt for license information. | ||
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception | ||
// | ||
//===----------------------------------------------------------------------===// | ||
|
||
#include "llvm/AsmParser/AsmParserContext.h" | ||
|
||
namespace llvm { | ||
|
||
std::optional<FileLocRange> | ||
AsmParserContext::getFunctionLocation(const Function *F) const { | ||
if (!Functions.contains(F)) | ||
|
||
return std::nullopt; | ||
return Functions.at(F); | ||
} | ||
|
||
std::optional<FileLocRange> | ||
AsmParserContext::getBlockLocation(const BasicBlock *BB) const { | ||
if (!Blocks.contains(BB)) | ||
return std::nullopt; | ||
return Blocks.at(BB); | ||
|
||
} | ||
|
||
std::optional<FileLocRange> | ||
AsmParserContext::getInstructionLocation(const Instruction *I) const { | ||
if (!Instructions.contains(I)) | ||
return std::nullopt; | ||
return Instructions.at(I); | ||
} | ||
|
||
Function * | ||
AsmParserContext::getFunctionAtLocation(const FileLocRange &Query) const { | ||
for (auto &[F, Loc] : Functions) { | ||
if (Loc.contains(Query)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This looks very inefficient... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's actually not that bad. When testing this even on large files the LSP was pretty responsive. So I wouldn't consider this an issue for now. |
||
return F; | ||
} | ||
return nullptr; | ||
} | ||
|
||
Function *AsmParserContext::getFunctionAtLocation(const FileLoc &Query) const { | ||
return getFunctionAtLocation(FileLocRange(Query, Query)); | ||
} | ||
|
||
BasicBlock * | ||
AsmParserContext::getBlockAtLocation(const FileLocRange &Query) const { | ||
for (auto &[BB, Loc] : Blocks) { | ||
if (Loc.contains(Query)) | ||
return BB; | ||
} | ||
return nullptr; | ||
} | ||
|
||
BasicBlock *AsmParserContext::getBlockAtLocation(const FileLoc &Query) const { | ||
return getBlockAtLocation(FileLocRange(Query, Query)); | ||
} | ||
|
||
Instruction * | ||
AsmParserContext::getInstructionAtLocation(const FileLocRange &Query) const { | ||
for (auto &[I, Loc] : Instructions) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Iterating over all instructions seems rather inefficient e.g. if you're trying to get info towards the end of large file. Can't we have a list sorted by file location and do a binary search? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it doesn't matter if the lookup by function is more common. Do you have a link to code using this new class? I'm not worried about performance for the LSP, optimizations can always be done later. Please don't interpret my review comments as negative towards this change, I am very excited to see this support land eventually. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have a draft PR with the LSP server #161969 |
||
if (Loc.contains(Query)) | ||
return I; | ||
} | ||
return nullptr; | ||
} | ||
|
||
Instruction * | ||
AsmParserContext::getInstructionAtLocation(const FileLoc &Query) const { | ||
return getInstructionAtLocation(FileLocRange(Query, Query)); | ||
} | ||
|
||
bool AsmParserContext::addFunctionLocation(Function *F, | ||
const FileLocRange &Loc) { | ||
return Functions.insert({F, Loc}).second; | ||
} | ||
|
||
bool AsmParserContext::addBlockLocation(BasicBlock *BB, | ||
const FileLocRange &Loc) { | ||
return Blocks.insert({BB, Loc}).second; | ||
} | ||
|
||
bool AsmParserContext::addInstructionLocation(Instruction *I, | ||
const FileLocRange &Loc) { | ||
return Instructions.insert({I, Loc}).second; | ||
} | ||
|
||
} // namespace llvm |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
# AsmParser | ||
add_llvm_component_library(LLVMAsmParser | ||
AsmParserContext.cpp | ||
LLLexer.cpp | ||
LLParser.cpp | ||
Parser.cpp | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Omit file name and emacs marker in new files.