[PATCH] D149079: [VPlan] Record IR flags on VPWidenRecipe directly (NFC).

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1077
       // load/store. Collect recipe if its underlying instruction has
       // poison-generating flags.
       Instruction *Instr = CurRec->getUnderlyingInstr();
----------------
Ayal wrote:
> Update comment - recipes that support flags can drop them now rather than be collected to have them dropped later. This suggests that recipeBase should indicate if it supports flags or not? In the long run, any recipe for an underlying having poison generating flags should be capable of dropping them, right?
Added a comment. 

With the current system, recipes which have IR flags can be casted to `VPRecipeWithIRFlags`, similar how IR instructions/operators can be casted to `OverflowingBinaryOperator` & co. D149082 migrates VPWidenGEPRecipe and D150027 migrates VPRecplicateRecipe and removes `MayGeneratePoisonRecipes`


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1007
+  /// Drop all poison-generating flags.
+  void dropPoisonGeneratingFlags() {
+    HasNUW = false;
----------------
Ayal wrote:
> Can drop only relevant flags according to OpType.
Updated, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1044
+
+  /// Add the enabled IR flags to \p I.
+  void transferFlags(Instruction *I) const {
----------------
Ayal wrote:
> This conceptually "sets" the IR flags of `I` rather than only adding those enabled? The two are identical iff `I` has (initially) all its flags unset. Cf. `copyIRFlags()`.
Changed the name to `setFlags` to make it clearer.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1054
+    } else if (OpType == OperationType::FPMathOp && getFastMathFlags().any())
+      I->setFastMathFlags(getFastMathFlags());
+  }
----------------
Ayal wrote:
> May as well set each flag of `I` directly here rather than first having getFastMathFlags() set each in FMFs and then copying all to `I`. Unless these flags are held in an FMF to be set directly.
updated to set them directly here.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:954
+public:
+  VPIRFlags(Instruction &I) {
+    if (auto *Op = dyn_cast<OverflowingBinaryOperator>(&I)) {
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > Record which of the three operators is recorded, to ensure appropriate flags are accessed?
> > > Initialize all to off?
> > Done. It looks like there's no easy way to check if an opcode on its own is a overflowing/possibly exact/FP op, so I added a helper enum class.
> An alternative may be to keep the OpType next to its flags, retaining a simple bit vector mask, which could be reset by a single zeroing of a short/int, as in:
> 
> ```
>   unsigned HasWrapFlags : 1;
>   unsigned HasNUW : 1;
>   unsigned HasNSW : 1;
> 
>   unsigned HasFastMathFlags : 1;
>   unsigned AllowReassoc : 1;
>   unsigned NoNaNs : 1;
>   unsigned NoInfs : 1;
>   unsigned NoSignedZeros : 1;
>   unsigned AllowReciprocal : 1;
>   unsigned AllowContract : 1;
>   unsigned ApproxFunc : 1;
> 
>   unsigned HasInBoundFlag : 1;
>   unsigned HasInBound : 1;
> ```
> 
> where "Other" corresponds to all "Has*Flags" (or everything) being false.
Updated to simply include it in the bitfield, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:972
+
+  /// Drop all poison-generating flags.
+  void dropPoisonGeneratingFlags() {
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > Ensuring the generated code will be treated as if via Instruction::dropPoisonGeneratingFlags().
> > > Can instead keep a flag in VPIRFlags indicating that such flags should be dropped, instead of replicating that logic here.
> > I am not sure how this would work when we don't have an `Instruction` available from which to drop. 
> > 
> > We could record a new flag when calling `dropPoisonGeneratingFlags` on a recipe, but this would then also require to call another function to call `Instruction::dropPoisonGeneratingFlags()` during code-gen. We still need to look up the flags to set during code-gen, so it seems cleaner to properly unset them here.
> > 
> > Also when thinking about printing the flags as part of the VPlan printing (which isn't the case yet)
> > 
> Agreed: better drop poison generating flags eagerly rather than record a dropPoison flag and (continuing to) drop them lazily.
> 
> A main motivation for trying to **reuse** existing FMF struct and dropPoisonGeneratingFlags() API  rather than **reimplement** them is to help keep the two in sync - which deserves a comment.
>  
> TL;DR: 
> The Instruction whose flags needs to drop will be available when generated, as the concluding part of transfer/copy flags, as done now.
> 
> Admittedly the flags still need to be recorded and looked up.
> 
> If a dropPoison flag is recorded, it should effect isExact() and hasNo*Wrap() for them to be consistent, so may as well flip their bit, or else inline them into transferFlags() - but they may be needed in the future, including by VPlan printing.
I added a note to `dropPoisonGeneratingFlags` that it needs to be kept in sync with `Instruction::dropPoisonGeneratingFlags`. 

 I think it is very unlikely that one of the existing flags will get re-classified as poison-generating. If a new poison-generating flag gets added to the IR, we will never set this flags unless it is added here. And then it should also be added to `dropPoisonGeneratingFlags`, so keeping things in sync should not be too much trouble (once all recipes migrated).

Printing is added in D150029. Please let me know if you still would prefer to separately track whether poison-generating flags need to be dropped.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:992
+    FMFs.setAllowContract(AllowContract);
+    FMFs.setApproxFunc(ApproxFunc);
+    return FMFs;
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > Better than having VPIRFlags hold FastMathFlags as an internal struct/class/union?
> > This is mostly to keep keep things tightly packed, but I can change that.
> Keeping things tightly packed looks great.
> 
> A union OTOH would emphasize that these are mutually exclusive alternatives, and also allow initializing all to zero at once.
> 
> The FMF flags are currently used only as a whole - to record and apply all using `setFastMathFlags(getFastMathFlags())`. I.e., access to individual flags is not needed nor provided. If/when such access is needed, it could and should be provided via getFastMathFlags(), as that provides their full API, but currently requires to first construct all FMFs, redundantly.
> WDYT?
Updated to use a union, thanks!

Also updated to set fast-math flags individually. I can also add individual getters.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1012
 /// where each ingredient transforms into a vectorized version of itself.
-class VPWidenRecipe : public VPRecipeBase, public VPValue {
+class VPWidenRecipe : public VPRecipeBase, public VPValue, public VPIRFlags {
+
----------------
Ayal wrote:
> fhahn wrote:
> > Ayal wrote:
> > > Should the recipe "have a" VPIRFlags rather than "be a" VPIRFlags?
> > The main benefit of inheriting is to make the common `transferFlags` and `dropPoisonGeneratingFlags` available. But the name `VPIRFlags` is not ideal for inheriting.. I changed it to `VPOpWithIRFlags`, but can also change the `VPWidenRecipe` (and others transitioned in future patches) to hold an instance instead.
> Class essentially holds IR flags, as in FMF (and more), so original name sounds fine as does having VPWidenRecipe hold an instance.
> Worth reasoning about how this aspect would relate to the class hierarchy of recipes now or as more are encountered?
> BTW, CmpInst::Predicate "flags" may also be supported in a similar way, cf. D50823
The current modeling is similar to `OverflwoingBinaryOperator`/`PossiblyExactOperator` in IR, providing a convenient way to check if an object may have flags, allowing for checking via `isa`/`dyn_cast`.

IMO CmpInst::Predicate is slightly different to the wrapping flags, as it is more like an (essential) argument, similar to designation type for casts.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149079