Page MenuHomePhabricator

[MIR][M68K] (Patch 2/8): Changes on Target-independent MIR part
ClosedPublic

Authored by myhsu on Sep 27 2020, 5:06 PM.

Details

Summary
  1. Use the new interface TargetInstrInfo::isRegisterOperandPCRel to tell MachineVerifier whether a specific register operand is legal to have OPERAND_PCREL. Since some of the pc-relative addressing modes in M68K involve (non-pc) registers.
  2. Make MachineBasicBlock::findDebugLoc to work on reverse_iterator.

Diff Detail

Event Timeline

myhsu created this revision.Sep 27 2020, 5:06 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2020, 5:06 PM
myhsu requested review of this revision.Sep 27 2020, 5:06 PM
jrtc27 added a subscriber: jrtc27.Sep 29 2020, 7:08 PM
jrtc27 added inline comments.
llvm/lib/CodeGen/MachineVerifier.cpp
1647–1648

I wonder whether this should be a TII (or similar) hook/field instead; weakening assertions for targets where the assertion is valid is a bit sad.

myhsu updated this revision to Diff 295462.Sep 30 2020, 9:58 PM

Fix formatting issues

myhsu updated this revision to Diff 296011.Oct 3 2020, 4:48 PM
myhsu edited the summary of this revision. (Show Details)

Add a new interface in TargetInstrInfo: isRegisterOperandPCRel to tell MachineVerifier whether specific register operand is legal to be pc-relative

myhsu marked an inline comment as done.Oct 3 2020, 4:48 PM
craig.topper added inline comments.
llvm/lib/CodeGen/MachineBasicBlock.cpp
1357

Is it possible MBBI is already at instr_rend() so that std::prev isn't valid?

rengolin added inline comments.Nov 17 2020, 2:58 AM
llvm/include/llvm/CodeGen/TargetInstrInfo.h
947

Shouldn't you also have added the derived implementations for each target?

myhsu added inline comments.Nov 17 2020, 4:19 PM
llvm/include/llvm/CodeGen/TargetInstrInfo.h
947

I'm not sure, is there a policy to enforce this? Because there is a default implementation here (i.e. return false) so functionally it works. Also some of the functions, the isCopyInstrImpl below for example, don't have implementations in every derived targets either

rengolin added inline comments.Nov 18 2020, 3:19 AM
llvm/include/llvm/CodeGen/TargetInstrInfo.h
947

There is no policy regarding this. Just now reading the MachineVerrifier again, it seems as this does not change the implementation for any other target that doesn't implement. No worries.

RKSimon added inline comments.Dec 9 2020, 2:38 AM
llvm/lib/CodeGen/MachineBasicBlock.cpp
1357

@myhsu Have you confirmed whether this can happen? It looks like it at least needs an assert, else an early-out.

myhsu updated this revision to Diff 310670.EditedDec 9 2020, 2:31 PM
  • Rebase to latest changes
  • Addressed feedbacks
llvm/lib/CodeGen/MachineBasicBlock.cpp
1357

sorry I missed this comment. I've just fixed it.

myhsu marked 4 inline comments as done.Dec 9 2020, 2:33 PM

This looks good to me but I'll let other people review again and approve. Thanks!

RKSimon accepted this revision.Dec 15 2020, 2:17 AM

LGTM - cheers

This revision is now accepted and ready to land.Dec 15 2020, 2:17 AM
jrtc27 added inline comments.Dec 20 2020, 9:49 AM
llvm/include/llvm/CodeGen/TargetInstrInfo.h
938–946
947

Current name doesn't make sense; the operand is PC-relative, but the question is whether that's a legal thing to have constructed.

myhsu updated this revision to Diff 320581.Feb 1 2021, 1:36 PM
myhsu marked 2 inline comments as done.
  • [NFC] Addressed feedbacks
jrtc27 accepted this revision.Feb 23 2021, 4:53 AM

Minor whitespace issues to fix otherwise fine by me.

llvm/include/llvm/CodeGen/MachineBasicBlock.h
840

Needs a blank line

851

Needs a blank line

llvm/lib/CodeGen/MachineBasicBlock.cpp
1338

Needs a blank line

1355

Needs a blank line

myhsu updated this revision to Diff 326502.Feb 25 2021, 2:16 PM
myhsu marked 4 inline comments as done.
  • [NFC] Addressed feedbacks
jrtc27 accepted this revision.Mar 3 2021, 6:06 AM
This revision was landed with ongoing or failed builds.Mar 8 2021, 12:33 PM
This revision was automatically updated to reflect the committed changes.