Page MenuHomePhabricator

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

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


  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.

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.

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

rengolin added inline comments.Tue, Nov 17, 2:58 AM

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

myhsu added inline comments.Tue, Nov 17, 4:19 PM

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.Wed, Nov 18, 3:19 AM

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.