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


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



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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:997
+  /// Add the enabled IR flags to \p I.
+  void transferFlags(Instruction *I) const {
+    if (isExact())
----------------
Ayal wrote:
> 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.)
Updated using the recorded type of the op, thanks!


================
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:
> 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.


================
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))
----------------
Ayal wrote:
> Retain comment somewhere?
I am not sure where a good place would be. I am not sure it is necessary, because now it it is sufficient to directly drop the flags in `collectPoisonGeneratingRecipes`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149079