[PATCH] D81558: [NewPM] Introduce PreserveCFG check

Thu Jun 11 02:26:08 PDT 2020

yrouban marked an inline comment as done.
yrouban added a comment.

In D81558#2085042 <https://reviews.llvm.org/D81558#2085042>, @fedor.sergeev wrote:

> I'm not sure if introducing base for standard instrumentation classes (Instrument/Instruments) is a good change or not.
>  What problem does it solve?
>  Anyway that part seems to be rather orthogonal to the checker itself.
>  I would like to have it separated - either introduce Instrument/Instruments thing before the checker and then use it,
>  //OR// implement the checker the same way as other standard instrumentations are done (just as separate members) and only then refactor it all into Instrument/Instruments.

Ok.
I do not like that the current design makes me expose of a new standard instrumentation class declaration for all users of //StandardInstrumentations//. I believe it can be done internally inside StandardInstrumentations.cpp. That is why I proposed the //Instrument// base class.

I'm still not sure that this patch is needed. So, please feel free to comment. I used this patch once to see if there are any PreservedCFG violating passes except InstCombine. I did not find any. This does not prove there is none, but some level of confidence it gives.

================
Comment at: llvm/lib/Passes/StandardInstrumentations.cpp:257
+class PreservedCFGChecker : public Instrument {
+  using CFG = DenseMap<const BasicBlock *, SmallVector<const BasicBlock *, 4>>;
+  SmallVector<Optional<CFG>, 8> GraphStackBefore;
----------------
I realized that it is unsafe to dereference any of the saved BasicBlock* at the validation check.
But derefrence is only needed to print the difference when we are goin to crash anyway.
Another problem is that we might think that there are no difference if CFG has changed but BasicBlock* happened to be the same. It seems this patch needs to track values.

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

https://reviews.llvm.org/D81558