[PATCH] D127729: [Bitcode] Support expanding constant expressions into instructions
Nicolai Hähnle via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Jun 21 03:29:47 PDT 2022
nhaehnle added a comment.
Most of this looks good to me, I just have a few nits and one bigger question about `phi` handling inline.
================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:99
+ "expand-constant-exprs", cl::init(false), cl::Hidden,
+ cl::desc("Expand constant expressions for testing purposes"));
+
----------------
Suggest to make it easier to understand for somebody who lacks context.
================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:520-524
+ uint8_t Opcode;
+ uint8_t Flags;
+ unsigned NumOperands;
+ unsigned Extra; // GEP inrange index or blockaddress BB id.
+ Type *SrcElemTy; // GEP source element type.
----------------
Shouldn't these be `private`?
================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:1421-1425
+ if (It == MaterializedValues.end()) {
+ Worklist.push_back(OpID);
+ break;
+ }
+ Ops.push_back(It->second);
----------------
Could be an if/else without a `break` so that all required values are pushed to the worklist immediately, which is slightly more convenient for the common case where the constant expression overall is a tree. But I don't feel strongly about it.
================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:5569-5582
+ if (!PhiConstExprBB->empty()) {
+ auto It = Args.find(BB);
+ if (It != Args.end()) {
+ // If this phi argument has already been expanded, reuse the
+ // previous value instead of materializing a new one.
+ V = It->second;
+ PhiConstExprBB->eraseFromParent();
----------------
I don't understand what `Args` is trying to achieve here. Each predecessor basic block should only appear once, so the `Args.find(BB)` should never be successful -- right?
I *think* perhaps what you're trying to do is optimize the case where multiple `phi` nodes have the same incoming value for the same basic block? But then the `Args` map would have to have bigger scope and look slightly different.
Speaking of, shouldn't the PhiConstExprBB be cached somehow so that multiple `phi` nodes use the same edge basic blocks?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D127729/new/
https://reviews.llvm.org/D127729
More information about the llvm-commits
mailing list