Page MenuHomePhabricator

WIP/RFC: Generic MachineInstr convenience wrappers.
AbandonedPublic

Authored by aemerson on Jul 9 2021, 10:15 AM.

Details

Summary

Instead of code patterns like:

if (MI.getOpcode() == TargetOpcode::G_LOAD)
  Ptr = MI.getOperand(1);

having some wrappers could make the opcode checking a bit shorter, and also allow us to define some abstractions over the raw MachineInstr specific to a particular generic operation. Here's an example of how that could work with G_LOAD, using automatic bool and MachineInstr* conversions.

Diff Detail

Unit TestsFailed

TimeTest
4,820 msx64 debian > MemProfiler-x86_64-linux-dynamic.TestCases::test_new_load_store.cpp
Script: -- : 'RUN: at line 5'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang --driver-mode=g++ -fmemory-profile -mno-omit-leaf-frame-pointer -fno-omit-frame-pointer -fno-optimize-sibling-calls -gline-tables-only -m64 -shared-libsan -O0 /var/lib/buildkite-agent/builds/llvm-project/compiler-rt/test/memprof/TestCases/test_new_load_store.cpp -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/compiler-rt/test/memprof/X86_64LinuxDynamicConfig/TestCases/Output/test_new_load_store.cpp.tmp

Event Timeline

aemerson created this revision.Jul 9 2021, 10:15 AM
aemerson requested review of this revision.Jul 9 2021, 10:15 AM
paquette added inline comments.Jul 9 2021, 10:26 AM
llvm/include/llvm/CodeGen/GlobalISel/GenericInstWrapper.h
42

I feel like "load reg" isn't really descriptive.

Maybe getDstReg() would be better? That would also make it easy to share terminology with other wrappers.

One additional use for this could be as a replacement for passing around MachineInstr when a specific operation is expected.

Register doFooOnLoad(MachineInstr &MI) {
  assert(MI.getOpcode() == TargetOpcode::G_LOAD);
  ...

could be:

Register dooFooOnLoad(GLoad Load) {
  Load.validate();
  ...
llvm/include/llvm/CodeGen/GlobalISel/GenericInstWrapper.h
42

Yeah, this is just a proposal, the specific names we could work out later (if we decide to do ahead at all).

arsenm added a comment.Jul 9 2021, 3:09 PM

I've thought we should have something that looks like llvm::Instruction and SDNode hiearachies so you can still do things like dyn_cast to a specific instruction type. Plus the current getOperand+getReg is really ugly given that most every operation can only use registers.

Can you change this to make dyn_cast work, rather than having to construct a wrapper class? I guess that would add a GMI subclass to MachineInstr, and then opcode specific subclasses of that.

I've thought we should have something that looks like llvm::Instruction and SDNode hiearachies so you can still do things like dyn_cast to a specific instruction type. Plus the current getOperand+getReg is really ugly given that most every operation can only use registers.

Can you change this to make dyn_cast work, rather than having to construct a wrapper class? I guess that would add a GMI subclass to MachineInstr, and then opcode specific subclasses of that.

I've thought we should have something that looks like llvm::Instruction and SDNode hiearachies so you can still do things like dyn_cast to a specific instruction type. Plus the current getOperand+getReg is really ugly given that most every operation can only use registers.

Can you change this to make dyn_cast work, rather than having to construct a wrapper class? I guess that would add a GMI subclass to MachineInstr, and then opcode specific subclasses of that.

Sure, I've made a separate patch for that discussion in D105751

aemerson abandoned this revision.Jul 12 2021, 5:20 PM