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.
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...
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.
|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.
|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.
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.
|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.
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.
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.