This is an archive of the discontinued LLVM Phabricator instance.

CodeGen: Use MachineInstr& in TargetInstrInfo, NFC
ClosedPublic

Authored by dexonsmith on Jun 25 2016, 7:19 AM.

Details

Summary

This is mostly a mechanical change to make TargetInstrInfo API take
MachineInstr& (instead of MachineInstr* or MachineBasicBlock::iterator)
when the argument is expected to be a valid MachineInstr. This is a
general API improvement.

Although it would be possible to do this one function at a time, that
would demand a quadratic amount of churn since many of these functions
call each other. Instead I've done everything as a block and just
updated what was necessary.

This is mostly mechanical fixes: adding and removing * and &
operators. The only non-mechanical change is to split
ARMBaseInstrInfo::getOperandLatencyImpl out from
ARMBaseInstrInfo::getOperandLatency. Previously, the latter took a
MachineInstr* which it updated to the instruction bundle leader; now,
the latter calls the former either with the same MachineInstr& or the
bundle leader.

As a side effect, this removes a bunch of MachineInstr* to
MachineBasicBlock::iterator implicit conversions, a necessary step
toward fixing PR26753.

Since this is such a monolithic change (and annoying to revert), I'd
appreciate if a couple of others could confirm that the API changes in
TargetInstrInfo.h are reasonable before I push this.

Diff Detail

Event Timeline

dexonsmith updated this revision to Diff 61889.Jun 25 2016, 7:19 AM
dexonsmith retitled this revision from to CodeGen: Use MachineInstr& in TargetInstrInfo, NFC.
dexonsmith updated this object.
dexonsmith added a subscriber: llvm-commits.
ab accepted this revision.Jun 25 2016, 9:42 AM
ab added a reviewer: ab.
ab added a subscriber: ab.

Everything LGTM, but you might want to wait for a third pair of eyes. A few inline comments.

Also, I see BPF doesn't need any changes; can you confirm?

include/llvm/Target/TargetInstrInfo.h
1197

Might as well fix NULL -> null/nullptr

lib/Target/AArch64/AArch64InstrInfo.cpp
834–835

Move this comment to the for loop below?

834–835

and remove B and do

if (To == To->getParent()->begin())

below?

1130–1131

Here and elsewhere: It's not new and unlikely to become a problem, but I'm a tad worried about eraseFromParent() on references (though we usually do nothing to prevent reusing erased pointers either, at least we could, I guess). WDYT?

lib/Target/AMDGPU/R600ExpandSpecialInstrs.cpp
116

Heh, I guess we already do that

This revision is now accepted and ready to land.Jun 25 2016, 9:42 AM
ab added a comment.Jun 25 2016, 9:47 AM
In D21726#467230, @ab wrote:

Also, I see BPF doesn't need any changes; can you confirm?

Looks like Lanai will though; I suppose that can be done separately.

MatzeB accepted this revision.Jun 25 2016, 12:09 PM
MatzeB added a reviewer: MatzeB.

I like these changes, thanks for doing all the work.

  • As expected not much to see in thediff, it is a mechanical change (with some code reformating).
  • This will bring some churn to people backporting patches and exploring version history but that is of course no good reason to stop this.
  • I assume we have a ubsanbot that catches references pointing to nullptr.

LGTM

lib/Target/AArch64/AArch64InstrInfo.cpp
1130–1131

I don't see a problem here. It would probably feel more natural as ˋMI.getParent().erase(MI);ˋ but that is not how we do things today...

lib/Target/AArch64/AArch64LoadStoreOptimizer.cpp
1091–1092

Could have been done separately, but +1 to not using auto when it is not extremely obvious.

lib/Target/AArch64/AArch64SchedCyclone.td
111

We should change the tablegen generated code to use MachineInstr&, but that is for another patch.

kparzysz accepted this revision.Jun 27 2016, 6:30 AM
kparzysz edited edge metadata.

The Hexagon part LGTM.

In D21726#467239, @ab wrote:
In D21726#467230, @ab wrote:

Also, I see BPF doesn't need any changes; can you confirm?

Looks like Lanai will though; I suppose that can be done separately.

Yes it would need to be updated. I can make those in a follow up if desired.

dexonsmith updated this revision to Diff 62164.Jun 28 2016, 6:56 PM
dexonsmith edited edge metadata.
  • Spell NULL as nullptr in moved comment, and clean up the areCFlagsAccessedBetweenInstrs helper in AArch64InstrInfo.cpp (responding to Ahmed's feedback).
  • Update off-by-default targets.
dexonsmith closed this revision.Jun 29 2016, 5:23 PM

Closing the phabricator, since I committed in r274193.