[PATCH] D104410: GlobalISel/Utils: Refactor constant splat match functions

[Petar Avramovic via Phabricator via llvm-commits]

Petar.Avramovic added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/Utils.cpp:367
 
+bool isGImplicitDef(const MachineInstr *MI) {
+  return MI->getOpcode() == TargetOpcode::G_IMPLICIT_DEF;
----------------
aemerson wrote:
> paquette wrote:
> > This will need a nullptr check.
> > 
> > I'm not sure about how I feel about adding functions for generic opcode checks, since I think it could end up being confusing if we don't add them for every opcode. :/
> > 
> > That being said, these do look nicer than using `MI->getOpcode()` everywhere.
> > 
> > (Maybe someone else can give a stronger opinion here.)
> I don't think alone this helper has much benefit. I think it would be nice to have some wrapper classes around MachineInstr to do some of that while also proving some abstractions over operand accessors etc. I haven't got around to it but was on my todo list.
I meant to add it for a number of opcodes (~50).
I did grep of '==' and '!=' compares  with 'TargetOpcode::G_' Here are some stats:
there are 44 different opcodes with 1 or 2 mentions,  in total 58 compares.
there are 49 different opcodes with more that 3 mentions (G_CONSTANT has the most; 30). In total 450 compares
190 are asserts.
Would result in nicer code formatting.
I will directly check opcode for this patch.


================
Comment at: llvm/lib/CodeGen/GlobalISel/Utils.cpp:367
 
+bool isGImplicitDef(const MachineInstr *MI) {
+  return MI->getOpcode() == TargetOpcode::G_IMPLICIT_DEF;
----------------
Petar.Avramovic wrote:
> aemerson wrote:
> > paquette wrote:
> > > This will need a nullptr check.
> > > 
> > > I'm not sure about how I feel about adding functions for generic opcode checks, since I think it could end up being confusing if we don't add them for every opcode. :/
> > > 
> > > That being said, these do look nicer than using `MI->getOpcode()` everywhere.
> > > 
> > > (Maybe someone else can give a stronger opinion here.)
> > I don't think alone this helper has much benefit. I think it would be nice to have some wrapper classes around MachineInstr to do some of that while also proving some abstractions over operand accessors etc. I haven't got around to it but was on my todo list.
> I meant to add it for a number of opcodes (~50).
> I did grep of '==' and '!=' compares  with 'TargetOpcode::G_' Here are some stats:
> there are 44 different opcodes with 1 or 2 mentions,  in total 58 compares.
> there are 49 different opcodes with more that 3 mentions (G_CONSTANT has the most; 30). In total 450 compares
> 190 are asserts.
> Would result in nicer code formatting.
> I will directly check opcode for this patch.
I like this better than util function.


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

https://reviews.llvm.org/D104410