[PATCH] D32789: [ScopInfo] Do not use LLVM names to identify statements

[Michael Kruse via Phabricator via llvm-commits]

Meinersbur added a comment.

Some post-commit remarks.



================
Comment at: polly/trunk/include/polly/ScopInfo.h:1714
+  /// The smallest array index not yet assigned.
+  long ArrayIdx = 0;
+
----------------
`long` is 32 bits on 32 bit system and any Windows system. If you really think 32 bits are not enough, could you consider using `long long` or `int64_t`?


================
Comment at: polly/trunk/include/polly/ScopInfo.h:2634
+  /// array.
+  long getNextArrayIdx() { return ArrayIdx++; }
+
----------------
This method has a getter-function name, but has side-effects.

Did you consider passing the ArrayIdx to `ScopArrayInfo`'s ctor instead?


================
Comment at: polly/trunk/lib/Support/GICHelper.cpp:218
+
+  if (UseInstructionNames && Val->hasName())
+    ValStr = std::string("_") + std::string(Val->getName());
----------------
Isn't making it also dependent on `Val->hasName()` a breaking/unrelated change? Before, the number was derived by LLVM taking all Value's into account. Now the id is derived from the order the unnamed value is encountered by Polly.

I wouldn't consider it a compile-time improvement either because LLVM still looks for duplicate name if it has a name. If `UseInstructionNames` is enabled, I'd expect names to be derived consistently by LLVM. It is also not consistent with the other overload and the function's description, which says it's only dependent on `UseInstructionNames`.


================
Comment at: polly/trunk/lib/Support/GICHelper.cpp:219
+  if (UseInstructionNames && Val->hasName())
+    ValStr = std::string("_") + std::string(Val->getName());
+  else
----------------
`ValStr = std::string("_") + Val->getName()` should suffice.


Repository:
  rL LLVM

https://reviews.llvm.org/D32789