Page MenuHomePhabricator

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

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

Unit TestsFailed

80 mslinux > LLVM.CodeGen/SystemZ::mverify-optypes.mir
Script: -- : 'RUN: at line 1'; not --crash /mnt/disks/ssd0/agent/llvm-project/build/bin/llc -mtriple=s390x-linux-gnu -mcpu=z14 -run-pass=none -o - /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/SystemZ/mverify-optypes.mir 2>&1 | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/SystemZ/mverify-optypes.mir

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.Nov 17 2020, 2:58 AM
947 ↗(On Diff #296011)

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

myhsu added inline comments.Nov 17 2020, 4:19 PM
947 ↗(On Diff #296011)

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
947 ↗(On Diff #296011)

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

@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

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
947–955 ↗(On Diff #310670)
956 ↗(On Diff #310670)

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.


Needs a blank line


Needs a blank line


Needs a blank line


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.