This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel][NFC] Add MachineInstr::getFirst[N]{Regs,LLTs}() helpers to extract regs & types.
ClosedPublic

Authored by aemerson on Feb 23 2023, 4:40 PM.

Details

Summary

These reduce the typing and clutter from:

Register Dst = MI.getOperand(0).getReg();
Register Src1 = MI.getOperand(1).getReg();
Register Src2 = MI.getOperand(2).getReg();
Register Src3 = MI.getOperand(3).getReg();
LLT DstTy = MRI.getType(Dst);
... etc etc

To just:

auto [Dst, Src1, Src2, Src3] = MI.getFirst4Regs();
auto [DstTy, Src1Ty, Src2Ty, Src3Ty] = MI.getFirst4LLTs();

Diff Detail

Event Timeline

aemerson created this revision.Feb 23 2023, 4:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 23 2023, 4:40 PM
aemerson requested review of this revision.Feb 23 2023, 4:40 PM
aemerson updated this revision to Diff 500080.Feb 23 2023, 11:05 PM
aemerson retitled this revision from [GlobalISel][NFC] Add some Get{2,3,4,5}RegsFromMI macros to extract regs from MachineInstrs. to [GlobalISel][NFC] Add some Get{2,3,4,5}RegsFromMI macros to extract regs & types from MachineInstrs.
aemerson edited the summary of this revision. (Show Details)

Add some variants that also declare the corresponding types for each register.

Could you achieve the same with structured bindings?

auto [Dst, Src1, Src2, Src3] = MI.getOperandsRegs(0, 1, 2, 3);
foad added a comment.Feb 24 2023, 1:56 AM

Could you achieve the same with structured bindings?

auto [Dst, Src1, Src2, Src3] = MI.getOperandsRegs(0, 1, 2, 3);

Yes I think that looks much nicer than the macros.

Could you achieve the same with structured bindings?

auto [Dst, Src1, Src2, Src3] = MI.getOperandsRegs(0, 1, 2, 3);

Yes that does seem much cleaner :)

aemerson updated this revision to Diff 500318.Feb 24 2023, 3:20 PM
aemerson retitled this revision from [GlobalISel][NFC] Add some Get{2,3,4,5}RegsFromMI macros to extract regs & types from MachineInstrs to [GlobalISel][NFC] Add MachineInstr::getFirst[N]{Regs,LLTs}() helpers to extract regs & types..
aemerson edited the summary of this revision. (Show Details)

Use structured bindings with helpers in MachineInstr.

foad added inline comments.Feb 28 2023, 2:10 AM
llvm/include/llvm/CodeGen/MachineInstr.h
1900

With CTAD you should be able to use std::tuple() constructor instead of std::make_tuple().

I’m also wondering for performance reasons whether we should have the types be returned via a single call. With the current patch we call getOperand() twice for each register and I’m not sure if they’ll get optimized to a single call. Maybe:
auto [DstReg, DstTy, Src1Reg, Src1Ty] = MI.getFirst2RegsAndTys();

I’m also wondering for performance reasons whether we should have the types be returned via a single call. With the current patch we call getOperand() twice for each register and I’m not sure if they’ll get optimized to a single call. Maybe:
auto [DstReg, DstTy, Src1Reg, Src1Ty] = MI.getFirst2RegsAndTys();

I’ve thought about adding a MachineValue that acts more like an SDValue which would track the type and index

aemerson added a comment.EditedFeb 28 2023, 10:55 PM

I’ve thought about adding a MachineValue that acts more like an SDValue which would track the type and index

I like the idea in principal, but I worry that it'll be tempting to pass those around as first class values, when they're moreso just ephemerally tied at the point in time. You could imagine a situation where the type of a register is mutated but references to MachineValue being held due to sharing end up stale. With separate registers and types it's harder to fall into that trap.

aemerson updated this revision to Diff 502077.Mar 3 2023, 1:42 AM

Address comments.

Also introduce getFirstNRegLLTs() which allow us to do both regs and types in pairs:

auto [Dst, DstTy, Src, SrcTy] = MI.getFirst2RegLLTs();
aemerson updated this revision to Diff 502078.Mar 3 2023, 1:45 AM

Fix build.

Seems good for me

arsenm added a comment.Mar 7 2023, 9:54 AM

I’ve thought about adding a MachineValue that acts more like an SDValue which would track the type and index

I like the idea in principal, but I worry that it'll be tempting to pass those around as first class values, when they're moreso just ephemerally tied at the point in time. You could imagine a situation where the type of a register is mutated but references to MachineValue being held due to sharing end up stale. With separate registers and types it's harder to fall into that trap.

I don’t really see a case where you can legally mutate a register type.

@arsenm do you have objections to this then? I don't feel too strongly either way.

arsenm accepted this revision.Apr 8 2023, 5:21 AM

Can look into MachineValue separately

This revision is now accepted and ready to land.Apr 8 2023, 5:21 AM