[PATCH] D33163: [Polly] Added the list of Instructions to output in ScopInfo pass

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

I cannot apply the patch. Can you rebase to the latest trunk?



================
Comment at: include/polly/ScopInfo.h:1225
+  /// Vector for Instructions in a Basic Block.
+  std::vector<Instruction *> BBToInst;
+
----------------
bollu wrote:
> Are there any uses of `BBToInst` other than to print the instructions? If not, can we change it to:
> 
> ```lang=cpp
> std::vector<const Instruction*> BBToInst;
> ```
> 
> If there are other uses where we mutate the `Instruction`, then this is perfectly fine.
Could you reconsider the name? `BBToInst` indicates a kind of map, and there are no BasicBlocks involed.

Please don't add `const`.
1. The instructions are going to be used for other things than printing them.
2. `llvm::Instruction` (or more generally, `llvm::Value`) is not a type intended to be passed `const`. Few functions, even not modifying the instruction take `const Instruction*` (such as ScalarEvolution::getSCEV)
3. `llvm::Instruction` is a type whose object's are identified by their address. That is, after modifying it (e.g. `replaceAllUsesWith` changes one of its operands), it is still considered the same instruction. `const` does not allow sich modifications.
4. `llvm::Instruction` uses intrusive linked links (such as `ilist_node_with_parent<Instruction, BasicBlock>`). That is, `const` inhibits changing in which such list the instruction is contained in.


================
Comment at: lib/Analysis/ScopBuilder.cpp:427
 
+  std::vector<Instruction *> BBToInst;
+
----------------
You can move this declaration into the `else` branch, avoiding it being initialized when not used.


https://reviews.llvm.org/D33163