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

[Ayal Zaks via Phabricator via llvm-commits]

Ayal added a comment.

Thanks! Looks good to me, adding last minor nits.



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1079
+      // collect them in a set.
+      // TODO: Migrate all recipes to hold their own flags.
       Instruction *Instr = CurRec->getUnderlyingInstr();
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/LoopVectorize.cpp:1082-1083
+      if (Instr && Instr->hasPoisonGeneratingFlags()) {
+        if (auto *OpWithFlags = dyn_cast<VPRecipeWithIRFlags>(CurRec))
+          OpWithFlags->dropPoisonGeneratingFlags();
+        else
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:940
 
+/// Class to record LLVM IR flag for a recipe.
+class VPRecipeWithIRFlags : public VPRecipeBase {
----------------



================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:949
+  };
+  struct FastMathFlagsTy {
+    char AllowReassoc : 1;
----------------
Can reuse FastMathFlags, but that would complicate initialization and turn the union from char  to int?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:967
+    FastMathFlagsTy FMFs;
+    char AllFlags;
+  };
----------------
Better as `unsigned char`?


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:976
+    HasExact = 0;
+    HasFastMathFlags = 0;
+    AllFlags = 0;
----------------
(0 >> false)
Ah, the purpose of having Has*Flags as indicator bits was to embed them with all other bits so that all could be initialized with one zeroing assignment, and they could direct relate to the bits they cover by placing them next to each other. Now an `enum` may (again) be preferred, referring to named structs, and possibly `switch`ed upon.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1034
+    return ExactFlags.IsExact;
+  }
+
----------------
These getters can be dropped after inlining them into their only use below in setFlags(), similar to how the latter accesses FMFs flags - directly w/o getters.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1036
+
+  FastMathFlags getFastMathFlags() const {
+    assert(HasFastMathFlags && "FMFs not available");
----------------
This getFastMathFlags() is now useless and can be dropped.
May be needed in the future, at which point it should also be tested.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1049
+
+  /// Set the enabled IR flags for \p I.
+  void setFlags(Instruction *I) const {
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149079