[PATCH] D139981: [EarlyIfConversion] Add target hook to allow for multiple ifcvt iterations.

[Hendrik Greving via Phabricator via llvm-commits]

hgreving added inline comments.


================
Comment at: llvm/lib/CodeGen/EarlyIfConversion.cpp:57
+// This bypasses all other heuristics, so it should be set fairly high.
+static cl::opt<bool> AllowPredicatingTwice(
+    "allow-predicating-twice", cl::init(false), cl::Hidden,
----------------
jroelofs wrote:
> hgreving wrote:
> > arsenm wrote:
> > > A command line flag isn’t the way to expose this. I would move this into a partner bit with isPredicable, maybe combining into a predicable kind field 
> > Thanks for the comment. The reason (I agree) I just added this switch is that it is a property / tweak of the pass, and only applies to archs where a predicable instruction "is predicated". On Hexagon, a predicted instruction (AFAICS, had to dig in more) is _not_ predicable. On x86, an instruction never returns isPredicated (I guess this makes sense).  ARM/AArch64 not 100% sure.
> > 
> > WDYT about a TII->canPredicatePredicatedIntrs() hook? Reason being this is more a property of the arch being able to support predicate logic rather than a property of each intruction.
> > 
> > Also problem is which architecture could I test this with :(?
> A TII hook seems appropriate. How about `TII->canPredicatePredicatedInstr(*I)`, so the interface doesn't need to change again when a target comes along that needs to decide per-instruction.
PTAL


================
Comment at: llvm/lib/CodeGen/EarlyIfConversion.cpp:57
+// This bypasses all other heuristics, so it should be set fairly high.
+static cl::opt<bool> AllowPredicatingTwice(
+    "allow-predicating-twice", cl::init(false), cl::Hidden,
----------------
hgreving wrote:
> jroelofs wrote:
> > hgreving wrote:
> > > arsenm wrote:
> > > > A command line flag isn’t the way to expose this. I would move this into a partner bit with isPredicable, maybe combining into a predicable kind field 
> > > Thanks for the comment. The reason (I agree) I just added this switch is that it is a property / tweak of the pass, and only applies to archs where a predicable instruction "is predicated". On Hexagon, a predicted instruction (AFAICS, had to dig in more) is _not_ predicable. On x86, an instruction never returns isPredicated (I guess this makes sense).  ARM/AArch64 not 100% sure.
> > > 
> > > WDYT about a TII->canPredicatePredicatedIntrs() hook? Reason being this is more a property of the arch being able to support predicate logic rather than a property of each intruction.
> > > 
> > > Also problem is which architecture could I test this with :(?
> > A TII hook seems appropriate. How about `TII->canPredicatePredicatedInstr(*I)`, so the interface doesn't need to change again when a target comes along that needs to decide per-instruction.
> PTAL
PTAL


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

https://reviews.llvm.org/D139981