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

[asb]

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.

[asb]

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]

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]

include/llvm/Target/Target.td

818 ↗

(On Diff #112375)

I would default hasSideEffects to 0.

[asb]

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]

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]

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]

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

[qcolombet]

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]

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]

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]

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]

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]

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]

Ping?

[asb]

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

[qcolombet]

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]

In D37065#904038, @qcolombet wrote:

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]

Sounds good to me.

LGTM with the additional TODOs comment on the side effects.

llvm/trunk/include/llvm/Target/Target.td

class InstrInfo {
   // generator has better support for complex operands and targets have
   // migrated away from using positionally encoded operands.
   bit noNamedPositionallyEncodedOperands = 0;
 }
 
 // Standard Pseudo Instructions.
 // This list must match TargetOpcodes.h and CodeGenTarget.cpp.
 // Only these instructions are allowed in the TargetOpcode namespace.
-let isCodeGenOnly = 1, isPseudo = 1, hasNoSchedulingInfo = 1,
-    Namespace = "TargetOpcode" in {
+// Ensure mayLoad and mayStore have a default value, so as not to break
+// targets that set guessInstructionProperties=0. Any local definition of
+// mayLoad/mayStore takes precedence over these default values.
+let mayLoad = 0, mayStore = 0, isCodeGenOnly = 1, isPseudo = 1,
+    hasNoSchedulingInfo = 1, Namespace = "TargetOpcode" in {
 def PHI : Instruction {
   let OutOperandList = (outs unknown:$dst);
   let InOperandList = (ins variable_ops);
   let AsmString = "PHINODE";
+  let hasSideEffects = 1;
 }
 def INLINEASM : Instruction {
   let OutOperandList = (outs);
   let InOperandList = (ins variable_ops);
   let AsmString = "";
   let hasSideEffects = 0;  // Note side effect is encoded in an operand.
 }
 def CFI_INSTRUCTION : Instruction {
   let OutOperandList = (outs);
   let InOperandList = (ins i32imm:$id);
   let AsmString = "";
   let hasCtrlDep = 1;
+  let hasSideEffects = 1;
   let isNotDuplicable = 0;
 }
 def EH_LABEL : Instruction {
   let OutOperandList = (outs);
   let InOperandList = (ins i32imm:$id);
   let AsmString = "";
   let hasCtrlDep = 1;
+  let hasSideEffects = 1;
   let isNotDuplicable = 1;
 }
 def GC_LABEL : Instruction {
   let OutOperandList = (outs);
   let InOperandList = (ins i32imm:$id);
   let AsmString = "";
   let hasCtrlDep = 1;
+  let hasSideEffects = 1;
   let isNotDuplicable = 1;
 }
 def ANNOTATION_LABEL : Instruction {
   let OutOperandList = (outs);
   let InOperandList = (ins i32imm:$id);
   let AsmString = "";
   let hasCtrlDep = 1;
+  let hasSideEffects = 1;
   let isNotDuplicable = 1;
 }
 def KILL : Instruction {
   let OutOperandList = (outs);
   let InOperandList = (ins variable_ops);
   let AsmString = "";
   let hasSideEffects = 0;
 }
def COPY : Instruction {
   let hasSideEffects = 0;
   let isAsCheapAsAMove = 1;
   let hasNoSchedulingInfo = 0;
 }
 def BUNDLE : Instruction {
   let OutOperandList = (outs);
   let InOperandList = (ins variable_ops);
   let AsmString = "BUNDLE";
+  let hasSideEffects = 1;
 }
 def LIFETIME_START : Instruction {
   let OutOperandList = (outs);
   let InOperandList = (ins i32imm:$id);
   let AsmString = "LIFETIME_START";
   let hasSideEffects = 0;
 }
 def LIFETIME_END : Instruction {
   let OutOperandList = (outs);
   let InOperandList = (ins i32imm:$id);
   let AsmString = "LIFETIME_END";
   let hasSideEffects = 0;
 }
 def STACKMAP : Instruction {
   let OutOperandList = (outs);
   let InOperandList = (ins i64imm:$id, i32imm:$nbytes, variable_ops);
+  let hasSideEffects = 1;
   let isCall = 1;
   let mayLoad = 1;
   let usesCustomInserter = 1;
 }
 def PATCHPOINT : Instruction {
   let OutOperandList = (outs unknown:$dst);
   let InOperandList = (ins i64imm:$id, i32imm:$nbytes, unknown:$callee,
                        i32imm:$nargs, i32imm:$cc, variable_ops);
+  let hasSideEffects = 1;
   let isCall = 1;
   let mayLoad = 1;
   let usesCustomInserter = 1;
 }
 def STATEPOINT : Instruction {
   let OutOperandList = (outs);
   let InOperandList = (ins variable_ops);
   let usesCustomInserter = 1;
def LOCAL_ESCAPE : Instruction {
   let InOperandList = (ins ptr_rc:$symbol, i32imm:$id);
   let hasSideEffects = 0;
   let hasCtrlDep = 1;
 }
 def FAULTING_OP : Instruction {
   let OutOperandList = (outs unknown:$dst);
   let InOperandList = (ins variable_ops);
   let usesCustomInserter = 1;
+  let hasSideEffects = 1;
   let mayLoad = 1;
   let mayStore = 1;
   let isTerminator = 1;
   let isBranch = 1;
 }
 def PATCHABLE_OP : Instruction {
   let OutOperandList = (outs unknown:$dst);
   let InOperandList = (ins variable_ops);

llvm/trunk/lib/Target/RISCV/RISCV.td

 
 def : ProcessorModel<"generic-rv64", NoSchedModel, [Feature64Bit]>;
 
 //===----------------------------------------------------------------------===//
 // Define the RISC-V target.
 //===----------------------------------------------------------------------===//
 
 def RISCVInstrInfo : InstrInfo {
-  // TODO: disable guessInstructionProperties when
-  // https://reviews.llvm.org/D37065 lands.
-  let guessInstructionProperties = 1;
+  let guessInstructionProperties = 0;
 }
 
 def RISCVAsmParser : AsmParser {
   let ShouldEmitMatchRegisterAltName = 1;
 }
 
 def RISCV : Target {
   let InstructionSet = RISCVInstrInfo;
   let AssemblyParsers = [RISCVAsmParser];
 }