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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal 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();
----------------
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?


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1044
+
+  /// Add the enabled IR flags to \p I.
+  void transferFlags(Instruction *I) const {
----------------
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()`.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1054
+    } else if (OpType == OperationType::FPMathOp && getFastMathFlags().any())
+      I->setFastMathFlags(getFastMathFlags());
+  }
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:954
+public:
+  VPIRFlags(Instruction &I) {
+    if (auto *Op = dyn_cast<OverflowingBinaryOperator>(&I)) {
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:972
+
+  /// Drop all poison-generating flags.
+  void dropPoisonGeneratingFlags() {
----------------
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.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:992
+    FMFs.setAllowContract(AllowContract);
+    FMFs.setApproxFunc(ApproxFunc);
+    return FMFs;
----------------
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?


================
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 {
+
----------------
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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149079