[PATCH] D37065: Ensure standard pseudo instructions (TargetOpcode::*) are compatible with guessInstructionProperties=0

[Quentin Colombet via Phabricator via llvm-commits]

qcolombet added inline comments.


================
Comment at: include/llvm/Target/Target.td:818
   let AsmString = "PHINODE";
+  let hasSideEffects = 1;
 }
----------------
asb wrote:
> qcolombet wrote:
> > asb wrote:
> > > qcolombet wrote:
> > > > I would default hasSideEffects to 0.
> > > 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.
> > Hmm, interesting, I wouldn't have expected this.
> > Could you elaborate on what are those?
> 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.
The MIR is definitely broken here :).
Could you investigate what breaks the IR and why the hasSideEffects bit affects that?


https://reviews.llvm.org/D37065