Page MenuHomePhabricator

Ensure standard pseudo instructions (TargetOpcode::*) are compatible with guessInstructionProperties=0
ClosedPublic

Authored by asb on Aug 23 2017, 8:09 AM.

Details

Summary

rL162640 introduced CodeGenTarget::guessInstructionProperties. If a target sets guessInstructionProperties=0 in its FootgtInstrInfo, tablegen will error if it has to guess properties from patterns. Unfortunately, this can't be used with current upstream LLVM as instructions in the TargetOpcode namespace are always included and have inferred properties for mayLoad and mayStore. This patch provides the simplest possible fix to this problem, defaulting mayLoad and mayStore in the TargetOpcode scope. The local definitions take precedence, so definitions like G_LOAD or PatchPoint have their expected properties and there is no functional change. This patch also adds hasSideEffects definitions to those instructions where it is currently inferred.

Diff Detail

Repository
rL LLVM

Event Timeline

asb created this revision.Aug 23 2017, 8:09 AM
asb retitled this revision from [RFC] Stop GenericOpcodes.td from breaking targets that set guessInstructionProperties=0 to [RFC][GlobalISel] Stop GenericOpcodes.td from breaking targets that set guessInstructionProperties=0.
asb added a comment.Aug 23 2017, 8:26 AM

It seems there are a number of other instructions in Target.td that also have this problem. guessInstructionProperties can't be enabled for a target without patching target-independent code, and this patch was incorrect to suggest GlobalISel was the only culprit. Let me look at this s ome more...

asb updated this revision to Diff 112375.Aug 23 2017, 8:52 AM
asb retitled this revision from [RFC][GlobalISel] Stop GenericOpcodes.td from breaking targets that set guessInstructionProperties=0 to Ensure standard pseudo instructions (TargetOpcode::*) are compatible with guessInstructionProperties=0.
asb edited the summary of this revision. (Show Details)
asb added a reviewer: stoklund.

The initial version of this patch wrongly focused on GenericOpcodes.td. Updated to tackle the problem head-on in Target.td. I've removed the RFC label as the updated version of the patch seems less likely to be controversial.

qcolombet added inline comments.Aug 23 2017, 9:45 AM
include/llvm/Target/Target.td
818 ↗(On Diff #112375)

I would default hasSideEffects to 0.

asb added inline comments.Aug 23 2017, 10:14 AM
include/llvm/Target/Target.td
818 ↗(On Diff #112375)

It seemed odd to me too, but changing it would mean this patch does represent a functional change. I did try setting it to 0 before updating this patch, but found it results in a number of MachineVerifier errors as well as changes in test output.

qcolombet added inline comments.Aug 23 2017, 10:26 AM
include/llvm/Target/Target.td
818 ↗(On Diff #112375)

Hmm, interesting, I wouldn't have expected this.
Could you elaborate on what are those?

asb added inline comments.Aug 23 2017, 11:10 AM
include/llvm/Target/Target.td
818 ↗(On Diff #112375)

There are 62 unexpected failures across the default targets. These see either differences in generated code (I haven't examined these), or fail the machine verifier with "Bad machine code: PHI operand is not in the CFG" as well as "Missing PHI operand". For former is triggered by the following logic in MachineVerifier::visitMachineOperand:

case MachineOperand::MO_MachineBasicBlock:
  if (MI->isPHI() && !MO->getMBB()->isSuccessor(MI->getParent()))
    report("PHI operand is not in the CFG", MO, MONum);

test/CodeGen/AArch64/swifterror.ll is an example of one of the smaller test cases that fails. It errors with:

# After Machine code sinking
# Machine code for function foo_loop: IsSSA, TracksLiveness
Function Live Ins: %X21 in %vreg0, %W0 in %vreg1, %S0 in %vreg2

BB#0: derived from LLVM BB %entry
    Live Ins: %X21 %W0 %S0
	%vreg2<def> = COPY %S0; FPR32:%vreg2
	%vreg1<def> = COPY %W0; GPR32:%vreg1
	%vreg0<def> = COPY %X21; GPR64:%vreg0
	%vreg4<def> = MOVi32imm 16; GPR32:%vreg4
	%vreg5<def> = SUBREG_TO_REG 0, %vreg4, sub_32; GPR64all:%vreg5 GPR32:%vreg4
	%vreg7<def> = MOVi32imm 1; GPR32:%vreg7
	%vreg8<def> = FMOVSi 112; FPR32:%vreg8
    Successors according to CFG: BB#1(?%)

BB#1: derived from LLVM BB %bb_loop
    Predecessors according to CFG: BB#0 BB#3
	CBNZW %vreg1, <BB#2>; GPR32:%vreg1
    Successors according to CFG: BB#2(0x50000000 / 0x80000000 = 62.50%) BB#5(0x30000000 / 0x80000000 = 37.50%)

BB#5: 
    Predecessors according to CFG: BB#1
	%vreg12<def> = PHI %vreg0, <BB#0>, %vreg11, <BB#3>; GPR64all:%vreg12,%vreg11 GPR64:%vreg0
	B <BB#3>
    Successors according to CFG: BB#3(?%)

BB#2: derived from LLVM BB %gen_error
    Predecessors according to CFG: BB#1
	ADJCALLSTACKDOWN 0, 0, %SP<imp-def,dead>, %SP<imp-use>
	%X0<def> = COPY %vreg5; GPR64all:%vreg5
	BL <ga:@malloc>, <regmask %FP %LR %B8 %B9 %B10 %B11 %B12 %B13 %B14 %B15 %D8 %D9 %D10 %D11 %D12 %D13 %D14 %D15 %H8 %H9 %H10 %H11 %H12 %H13 %H14 %H15 %S8 %S9 %S10 %S11 %S12 %S13 %S14 and 57 more...>, %LR<imp-def,dead>, %SP<imp-use>, %X0<imp-use>, %SP<imp-def>, %X0<imp-def>
	ADJCALLSTACKUP 0, 0, %SP<imp-def,dead>, %SP<imp-use>
	%vreg6<def> = COPY %X0; GPR64sp:%vreg6
	%vreg3<def> = COPY %vreg6; GPR64all:%vreg3 GPR64sp:%vreg6
	STRBBui %vreg7, %vreg6, 8; mem:ST1[%tmp] GPR32:%vreg7 GPR64sp:%vreg6
    Successors according to CFG: BB#3(?%)

BB#3: derived from LLVM BB %bb_cont
    Predecessors according to CFG: BB#2 BB#5
	%vreg11<def> = PHI %vreg12, <BB#5>, %vreg3, <BB#2>; GPR64all:%vreg11,%vreg12,%vreg3
	FCMPSrr %vreg2, %vreg8, %NZCV<imp-def>; FPR32:%vreg2,%vreg8
	Bcc 13, <BB#1>, %NZCV<imp-use>
	B <BB#4>
    Successors according to CFG: BB#4(0x04000000 / 0x80000000 = 3.12%) BB#1(0x7c000000 / 0x80000000 = 96.88%)

BB#4: derived from LLVM BB %bb_end
    Predecessors according to CFG: BB#3
	%vreg9<def> = COPY %vreg11; GPR64all:%vreg9,%vreg11
	%vreg10<def> = FMOVS0; FPR32:%vreg10
	%S0<def> = COPY %vreg10; FPR32:%vreg10
	%X21<def> = COPY %vreg9; GPR64all:%vreg9
	RET_ReallyLR %S0<imp-use>, %X21<imp-use>

# End machine code for function foo_loop.

*** Bad machine code: PHI operand is not in the CFG ***
- function:    foo_loop
- basic block: BB#5  (0x2331780)
- instruction: %vreg12<def> = PHI
- operand 2:   <BB#0>

*** Bad machine code: PHI operand is not in the CFG ***
- function:    foo_loop
- basic block: BB#5  (0x2331780)
- instruction: %vreg12<def> = PHI
- operand 4:   <BB#3>

*** Bad machine code: Missing PHI operand ***
- function:    foo_loop
- basic block: BB#5  (0x2331780)
- instruction: %vreg12<def> = PHI
BB#1 is a predecessor according to the CFG.
LLVM ERROR: Found 3 machine code errors.

Backends with failing test cases include X86, AArch64, PowerPC, ARM, AMDGPU, Hexagon, and SystemZ. Interestingly, Mips sees no failures.

asb updated this revision to Diff 112447.Aug 23 2017, 2:21 PM

The explanatory comment had the wrong polarity for guessInstructionProperties. This update fixes that.

qcolombet added inline comments.Aug 23 2017, 2:58 PM
include/llvm/Target/Target.td
818 ↗(On Diff #112375)

The MIR is definitely broken here :).
Could you investigate what breaks the IR and why the hasSideEffects bit affects that?

asb added a comment.Aug 24 2017, 1:26 AM

Hi Quentin, D37097 sets hasSideEffects=0 for PHI and fixes the passes that relied on the old value. I've separated out that change, as I think it makes most sense to commit this patch which just makes the currently inferred flags explicit, then to follow up and fix the flags that were questionable.

Let me know if you're happy for this first step to land.

qcolombet added inline comments.Aug 24 2017, 5:02 PM
include/llvm/Target/Target.td
831 ↗(On Diff #112447)

Should be 0 IMHO

839 ↗(On Diff #112447)

Ditto

847 ↗(On Diff #112447)

Ditto

915 ↗(On Diff #112447)

Ditto

asb added a subscriber: tstellar.Aug 25 2017, 5:37 AM
asb added inline comments.
include/llvm/Target/Target.td
915 ↗(On Diff #112447)

This leads to changes in test output in several AMDGPU tests as well as ARM/Windows/tls.ll. After setting hasSideEffects to 0, MachineInstr::hasUnmodeledSideEffects will return true for a bundle only if that property holds for one of its member instructions. From a cursory look, the changed code didn't appear to be broken. I'll rustle up a patch that sets hasSideEffects=0 on bundle, but @tstellar please do chime in if you have any concerns about BUNDLE having hasSideEffects=0.

tstellar added inline comments.Aug 25 2017, 6:28 AM
include/llvm/Target/Target.td
915 ↗(On Diff #112447)

This doesn't seem like it should be an issue, but I'd have to see what the test changes look like.

asb updated this revision to Diff 118608.Oct 11 2017, 7:07 AM

ANNOTATION_LABEL was added since this patch was produced. I've updated it so it also explicitly sets hasSideEffects.

Can someone confirm they're happy for this to land? This patch maintains the status quo, i.e. explicitly setting hasSideEffects where it would currently be inferred. The child/dependent patches change hasSideEffects to 0 where it makes sense.

asb updated this revision to Diff 119906.Oct 23 2017, 11:07 AM

The change to Lanai.td should not have been included. Refreshing patch.

qcolombet edited edge metadata.Oct 23 2017, 11:44 AM

Hi Alex,

I see that we stuck to hasSideEffects = 1 for BUNDLE, EH_LABEL and so on (the ones I marked with Ditto at some point).
Are we going to do the same thing as PHIs here, or do we plan to leave them with that flag ON?

I am not sure it is the right thing to do, but assuming it is, could you add a comment before each of them to explain why they have side effects.

Cheers,
-Quentin

asb added a comment.Oct 23 2017, 11:51 AM

Hi Alex,

I see that we stuck to hasSideEffects = 1 for BUNDLE, EH_LABEL and so on (the ones I marked with Ditto at some point).
Are we going to do the same thing as PHIs here, or do we plan to leave them with that flag ON?

I am not sure it is the right thing to do, but assuming it is, could you add a comment before each of them to explain why they have side effects.

My intention is to commit this as a (hopefully) no-op change, then land further patches which change the current behaviour to the desired behaviour (see D37097 and D37230). The only problem with the BUNDLE update is that I find it very time consuming to craft and verify updates to the AMDGPU tests, so can't guarantee I'll have the time to finish it off. I'm committed to landing patches to the other instructions though.

I'll update this patch to add FIXMEs/TODOs for the instructions we believe should have hasSideEffects = 0.

qcolombet accepted this revision.Oct 23 2017, 3:34 PM

Sounds good to me.

LGTM with the additional TODOs comment on the side effects.

This revision is now accepted and ready to land.Oct 23 2017, 3:34 PM
This revision was automatically updated to reflect the committed changes.