This is an archive of the discontinued LLVM Phabricator instance.

GlobalISel: Introduce GenericMachineInstr classes and derivatives for idiomatic LLVM RTTI.
ClosedPublic

Authored by aemerson on Jul 9 2021, 3:52 PM.

Details

Summary

This adds some level of type safety, allows helper functions to be added for specific opcodes for free, and also allows us to succinctly check for class membership with the usual dyn_cast/isa/cast functions.

To start off with, add variants for the different load/store operations with some places using it.

Diff Detail

Event Timeline

aemerson created this revision.Jul 9 2021, 3:52 PM
aemerson requested review of this revision.Jul 9 2021, 3:52 PM

I think I like this direction more just because the casting stuff is more consistent with the rest of LLVM. That might be more approachable for people who are more experienced with other parts of LLVM.

I think I like this direction more just because the casting stuff is more consistent with the rest of LLVM. That might be more approachable for people who are more experienced with other parts of LLVM.

+1 from me.

tschuett added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/GenericMachineInstrs.h
25

Does this need to be public?

30

Can this be protected?

aemerson added inline comments.Jul 12 2021, 9:59 AM
llvm/include/llvm/CodeGen/GlobalISel/GenericMachineInstrs.h
25

Yes, all the public members of MachineInstr need to be accessible.

30

Likewise, this also needs to be accessible outside.

aemerson updated this revision to Diff 358018.Jul 12 2021, 11:58 AM

Convert some explicit checking of MI.getOpcode() to use the new types.

Also add a templated variant of getOpcodeDef() that takes the class to match instead of an opcode.

aemerson retitled this revision from WIP/RFC: Introduce GenericMachineInstr classes and derivatives for idiomatic LLVM RTTI. to GlobalISel: Introduce GenericMachineInstr classes and derivatives for idiomatic LLVM RTTI..Jul 12 2021, 11:59 AM
aemerson edited the summary of this revision. (Show Details)
arsenm added inline comments.Jul 12 2021, 12:12 PM
llvm/include/llvm/CodeGen/GlobalISel/GenericMachineInstrs.h
25

Can we get away with allocating these as regular MachineInstrs since there are no real members?

arsenm added inline comments.Jul 12 2021, 12:26 PM
llvm/include/llvm/CodeGen/GlobalISel/GenericMachineInstrs.h
62

Also getPtrReg? A get-single-mmo wrapper would be nice too

113

Other places refer to this as the value reg

aemerson added inline comments.Jul 12 2021, 3:06 PM
llvm/include/llvm/CodeGen/GlobalISel/GenericMachineInstrs.h
25

Not sure what you mean. Have the others inherit directly from MachineInstr? If so we wouldn't be able to declare GISel helpers like getReg(), unless we add those to MachineInstr. Even then, it would be useful to have this to enforce more type safety in future for places which only expect to deal with generic instructions.

arsenm added inline comments.Jul 12 2021, 3:24 PM
llvm/include/llvm/CodeGen/GlobalISel/GenericMachineInstrs.h
25

I mean is it legal C++ to do:

X = new MachineInstr()
Y = cast<GenericMachineInstr>(X)

with guaranteed sizeof(*X) == sizeof(*Y), or would we need to allocate these initially as GenericMachineInstr?

aemerson added inline comments.Jul 12 2021, 3:37 PM
llvm/include/llvm/CodeGen/GlobalISel/GenericMachineInstrs.h
25

Yes that's legal as long as we don't store any data in GenericMachineInstr or its derivatives. I think the same mechanism is already used in the IR, e.g. IntrinsicInst is a convenience class around CallInst that just provides classof and other helpers, like we're doing here.

aemerson added inline comments.Jul 12 2021, 3:39 PM
llvm/include/llvm/CodeGen/GlobalISel/GenericMachineInstrs.h
62

Already provided in the base class GLoadStore.

I'll add a getFirstMMO() helper. We can also add more things like isAtomic() and isVolatile().

arsenm added inline comments.Jul 12 2021, 3:40 PM
llvm/include/llvm/CodeGen/GlobalISel/GenericMachineInstrs.h
62

Only one is legal, so no first

aemerson updated this revision to Diff 358096.Jul 12 2021, 4:28 PM

Add:

getMMO()
getMemSizeInBits()
getMemSize()
isAtomic()
isVolatile()

and use where possible.

Maybe an isSimple() would be handy too?

(There are some combines in the DAGCombiner which use a similarly-named function.)

aemerson updated this revision to Diff 358105.Jul 12 2021, 5:06 PM

Add isSimple(), and rename GStore::getStoredReg() -> GStore::getValueReg()

arsenm added inline comments.Jul 13 2021, 5:00 PM
llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
600

Why isn't this GAnyLoad?

aemerson added inline comments.Jul 13 2021, 8:49 PM
llvm/include/llvm/CodeGen/GlobalISel/CombinerHelper.h
600

To retain existing behaviour, this function only matches zero extending loads. I wanted to have the function signature reflect the restricted opcode we expect to have returned.

paquette accepted this revision.Jul 15 2021, 10:38 AM

I think this looks fine at this point. Added a couple nits but that's about it.

llvm/lib/CodeGen/GlobalISel/CombinerHelper.cpp
701–702

This comment seems redundant now.

701–702

Now that we don't need to pull out MMO explicitly, I think this can be folded into the previous if.

This revision is now accepted and ready to land.Jul 15 2021, 10:38 AM
This revision was landed with ongoing or failed builds.Jul 15 2021, 3:22 PM
This revision was automatically updated to reflect the committed changes.