[PATCH] D108573: [VPlan] Introduce code to limit querying VPValues using IR references.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:9489
+  // in ways that accessing values using original IR values is incorrect.
+  Plan->markIRToVPlanDisallowed();
+
----------------
Ayal wrote:
> Perhaps refactor to have buildVPlanWithVPRecipes() take care of building the initial unoptimized VPlan only, ending with marking IRToVPlan disallowed. I.e., invoke sinkScalarOperands() and mergeReplicateRegion() optimizations after buildVPlanWithVPRecipes() returns?
I'm not sure, introduction of `FirstOrderRecurrenceSplice` could also be moved after disallowing Value2VPValue access, but it is not an optimization. Should we do the potential split separately, to keep the diff smaller?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:2149
+  /// is not safe any longer.
+  void markIRToVPlanDisallowed() { IRToVPlanAllowed = false; }
+
----------------
Ayal wrote:
> `disableGetVPValue()`?
> 
> Some related thoughts: should addVPValue() and removeVPValue() also be disallowed, i.e., disable all accesses to Value2VPValue? Conceptually, Value2VPValue can be destroyed, and potentially another type of VPlan which does not support Value2VPValue created and provided instead.
> Some related thoughts: should addVPValue() and removeVPValue() also be disallowed, i.e., disable all accesses to Value2VPValue

Sounds good! I added assertions there and renamed the code to use `disableValue2VPValue`/`Value2VPValueEnabled`.

> Conceptually, Value2VPValue can be destroyed, and potentially another type of VPlan which does not support Value2VPValue created and provided instead.

Agreed, once the offending pieces of code have been removed, we can clear the map instead.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108573