[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/VPlan.h:954
+public:
+  VPIRFlags(Instruction &I) {
+    if (auto *Op = dyn_cast<OverflowingBinaryOperator>(&I)) {
----------------
Record which of the three operators is recorded, to ensure appropriate flags are accessed?
Initialize all to off?


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:992
+    FMFs.setAllowContract(AllowContract);
+    FMFs.setApproxFunc(ApproxFunc);
+    return FMFs;
----------------
Better than having VPIRFlags hold FastMathFlags as an internal struct/class/union?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:997
+  /// Add the enabled IR flags to \p I.
+  void transferFlags(Instruction *I) const {
+    if (isExact())
----------------
This replaces `Instruction::copyIRFlags(const Value *V, bool IncludeWrapFlags = true)` which sets the flags of "this" according to those of `V`, whether on or off, but does so in the opposite direction - setting flags of `I` according to those of "this", as in:
```
  if (isa<OverflowingBinaryOperator>(I)) {
    I->setHasNoSignedWrap(hasNoSignedWrap());
    I->setHasNoUnsignedWrap(hasNoUnsignedWrap());
  } else if (isa<PossiblyExactOperator>(I)) {
    I->setIsExact(isExact());
  } else if (isa<FPMathOperator>(I)) {
    I->copyFastMathFlags(getFastMathFlags());
  }
```
where each clause assumes or checks that VPIRFlags also records the same operator.
But it does so by only turning-on in `I` every flag that is turned-on in VPIRFlags - assuming are all initialized off in both VPIRFlags and `I`?

The current approach could use each turn-on flag to indicate which operator is recorded, i.e., could do
```
    if (isExact())
      I->setIsExact();
    else if (getFastMathFlags().any())
      I->setFastMathFlags(getFastMathFlags());
    else {
      if (hasNoUnsignedWrap())
        I->setHasNoUnsignedWrap();
      if (hasNoSignedWrap())
        I->setHasNoSignedWrap();
    }
```
(just to clarify understanding.)

(Note that GEPs should record an inBounds flag.)


================
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 {
+
----------------
Should the recipe "have a" VPIRFlags rather than "be a" VPIRFlags?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlanRecipes.cpp:640
-        // instruction is no longer guarded by the predicate, which could make
-        // the flag properties to no longer hold.
-        if (State.MayGeneratePoisonRecipes.contains(this))
----------------
Retain comment somewhere?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149079