[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:1079
+      // collect them in a set.
+      // TODO: Migrate all recipes to hold their own flags.
       Instruction *Instr = CurRec->getUnderlyingInstr();
----------------
Ayal wrote:
> 
Adjusted, thanks!


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


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


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:949
+  };
+  struct FastMathFlagsTy {
+    char AllowReassoc : 1;
----------------
Ayal wrote:
> Can reuse FastMathFlags, but that would complicate initialization and turn the union from char  to int?
Yeah I think that would make things more complicated unfortunately and unnecessarily increase the size for now.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:967
+    FastMathFlagsTy FMFs;
+    char AllFlags;
+  };
----------------
Ayal wrote:
> Better as `unsigned char`?
Updated, thanks!


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:976
+    HasExact = 0;
+    HasFastMathFlags = 0;
+    AllFlags = 0;
----------------
Ayal wrote:
> (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.
Switched back to the enum and updated code to use switch.


================
Comment at: llvm/lib/Transforms/Vectorize/VPlan.h:1034
+    return ExactFlags.IsExact;
+  }
+
----------------
Ayal wrote:
> 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.
Yep, removed them.


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149079