[ARM] Fix IT block generation after Thumb2SizeReduce with -Oz

[NickGuy]

Summary

Fixes a regression caused by D82439, in which IT blocks were no longer being
generated when -Oz is present. This was due to the CPSR register being marked as
dead, while this case was not accounted for.

Tests: 310
Metric: size..text

Program	Pre-patch	Post-patch	diff
test-suite.../Prolangs-C++/ocean/ocean.test	2168	2160	-0.4%
test-suite...s/Misc/richards_benchmark.test	1432	1428	-0.3%
test-suite...out-C++/Shootout-C++-hash.test	1696	1692	-0.2%
test-suite...ut-C++/Shootout-C++-hash2.test	1924	1920	-0.2%
test-suite.../Applications/spiff/spiff.test	12200	12176	-0.2%
test-suite.../Benchmarks/Ptrdist/ks/ks.test	2768	2764	-0.1%
test-suite...Source/Benchmarks/sim/sim.test	8712	8704	-0.1%
test-suite...s/MallocBench/cfrac/cfrac.test	12360	12352	-0.1%
test-suite...lFlow-dbl/ControlFlow-dbl.test	12896	12888	-0.1%
test-suite...chmarks/MallocBench/gs/gs.test	78160	78112	-0.1%
test-suite...lications/sqlite3/sqlite3.test	168436	168340	-0.1%
test-suite...telecomm-gsm/telecomm-gsm.test	15012	15004	-0.1%
test-suite...ediabench/gsm/toast/toast.test	15012	15004	-0.1%
test-suite...urce/Applications/hbd/hbd.test	18504	18496	-0.0%
test-suite...encode/alacconvert-encode.test	19924	19916	-0.0%
test-suite...decode/alacconvert-decode.test	19924	19916	-0.0%
test-suite.../Benchmarks/Ptrdist/bc/bc.test	20752	20744	-0.0%
test-suite...BLambdaLoops/lcalsBLambda.test	85764	85732	-0.0%
test-suite...pplications/oggenc/oggenc.test	90176	90144	-0.0%
test-suite...SubsetCRawLoops/lcalsCRaw.test	90532	90500	-0.0%
test-suite...CLambdaLoops/lcalsCLambda.test	90628	90596	-0.0%
test-suite...ks/Prolangs-C/agrep/agrep.test	23884	23876	-0.0%
test-suite...lFlow-flt/ControlFlow-flt.test	13384	13380	-0.0%
test-suite...lications/ClamAV/clamscan.test	245996	245924	-0.0%
test-suite...terchange/LoopInterchange.test	59128	59112	-0.0%
test-suite...eProcessing/Dilate/Dilate.test	60376	60360	-0.0%
test-suite...Filtering/BilateralFilter.test	60632	60616	-0.0%
test-suite...sion/AnisotropicDiffusion.test	60680	60664	-0.0%
test-suite...ImageProcessing/Blur/blur.test	61016	61000	-0.0%
test-suite...eProcessing/Dither/Dither.test	61304	61288	-0.0%
test-suite...urce/Applications/lua/lua.test	61520	61504	-0.0%
test-suite...yApps-C++/PENNANT/PENNANT.test	32276	32268	-0.0%
test-suite...ProxyApps-C++/CLAMR/CLAMR.test	217388	217340	-0.0%
test-suite...-typeset/consumer-typeset.test	279140	279084	-0.0%
test-suite.../Applications/SPASS/SPASS.test	201528	201488	-0.0%
test-suite...ications/JM/ldecod/ldecod.test	128356	128332	-0.0%
test-suite...SubsetBRawLoops/lcalsBRaw.test	85748	85732	-0.0%
test-suite...plications/d/make_dparser.test	43956	43964	0.0%
test-suite...SubsetARawLoops/lcalsARaw.test	88532	88516	-0.0%
test-suite...ALambdaLoops/lcalsALambda.test	89396	89380	-0.0%
test-suite...arks/mafft/pairlocalalign.test	168656	168632	-0.0%
test-suite...nsumer-jpeg/consumer-jpeg.test	60000	59992	-0.0%
test-suite...abench/jpeg/jpeg-6a/cjpeg.test	61920	61912	-0.0%
test-suite...ce/Benchmarks/PAQ8p/paq8p.test	40636	40632	-0.0%
test-suite...CI_Purple/SMG2000/smg2000.test	89536	89544	0.0%
test-suite...rks/tramp3d-v4/tramp3d-v4.test	198420	198404	-0.0%
test-suite...ications/JM/lencod/lencod.test	317976	317952	-0.0%
test-suite...Applications/kimwitu++/kc.test	241500	241492	-0.0%
test-suite.../Benchmarks/Bullet/bullet.test	296420	296412	-0.0%
test-suite...marks/7zip/7zip-benchmark.test	323580	323584	0.0%
test-suite...peg2/mpeg2dec/mpeg2decode.test	22968	22976	0.0%
test-suite...cations/hexxagon/hexxagon.test	7224	7228	0.1%
test-suite...s/Ptrdist/anagram/anagram.test	1940	1944	0.2%
test-suite...Adobe-C++/stepanov_vector.test	7888	7904	0.2%
test-suite...-C++/stepanov_abstraction.test	6340	6356	0.3%
Geomean difference			-0.0%

	Pre-Patch	Post-Patch	diff
count	310.000000	310.000000	310.000000
mean	19626.800000	19624.077419	-0.000062
std	48727.318461	48720.664877	0.000428
min	292.000000	292.000000	-0.003690
25%	1090.000000	1090.000000	0.000000
50%	2772.000000	2770.000000	0.000000
75%	10185.000000	10185.000000	0.000000
max	323580.000000	323584.000000	0.002524

[dmgreen]

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp

589–597

The definition of DefinesPredicate says this for it's doc:

/// If the specified instruction defines any predicate
/// or condition code register(s) used for predication, returns true as well
/// as the definition predicate(s) by reference.

I'm not sure we can just exclude dead predicates. They can still clobber the predicate in a way that might not be legal, and it looks like ifconvert is trying to use it for this reasons - to detect where the predicate might get clobbered.

There is this comment from ifconvert:

// FIXME: Make use of PredDefs? e.g. ADDC, SUBC sets predicates but are
// still potentially predicable.

Perhaps we have to tackle this in there? Or at least provide a way to remove the dead cpsr from the instruction. Perhaps just clearing the cpsr use from T1 instructions in PredicateInstruction would be enough?

llvm/test/CodeGen/Thumb2/constant-hoisting.ll

44

Having a T1 addseq inside an IT block isn't really legal I believe - it should become an T1 addeq (by dropping the cpsr def) or a T2 instruction. I think if you try outputting an object file and disassembling it, it would end up as a T1 addeq, but assembling this directly might give a thumb2 instruction?

That might not cause any bugs, per-se, both are correct. But is something we should probably try and get right, in the same way we strive to not be non-deterministic.

[NickGuy]

Addressed comments, and rebased to include D84653 (Which handles the IT-block instruction legality)

[dmgreen]

Thanks. LGTM with a very minor nitpick

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp

594

You needn't put { brackets on single statement if blocks.

[akhuang]

This is causing test failures / crashes in several chromium android tests (https://crbug.com/1114852). I'm not really familiar with this code, but it seems like after this change, there are some lines missing in the assembly.

Here's a diff of the object file for the test before and after this change: https://reviews.llvm.org/P8229

[NickGuy]

Hi @akhuang, thanks for bringing this up.

Would it be possible to provide a simple reproducer for this?

[akhuang]

I haven't been able to make a simple reproducer, but I attached a reproducer for compiling the object file here: https://crbug.com/1114852.

[dmgreen]

Thanks. It's hard to tell in all that where the problem might be, there are quite a few changes in all! And I didn't spot anything obvious by eyeballing it.

Is it possible to reproduce the original? Into a program that we can run ideally, and start figuring out where the problem in it is? Or would that still be a lot of steps?

If we are confident that this is the commit causing the problem (and it sounds like we are) then we can revert in the meantime. But we do need some way of starting to figure out how to fix it :)

[akhuang]

Sorry, I was hoping the object file diff would be clearer. If it could be reverted in the meantime, that would be helpful. I'm not sure if I'll be able to make an actual reproducer, but I'll at least try to narrow down the object file change.

[dmgreen]

^

How confident are we that the repo.cc file is the cause of the bug? As opposed to another object file? Like I said, this change unfortunately makes quite a few differences and it's a bit hard to tell which one is at fault at the moment! I don't think we saw anything fail locally from this (but I may have just missed it), so I'm trying to figure out how to pin this down.

[akhuang]

Actually, I've looked into it more and I think that isn't the correct object file. I'm bisecting to figure out which object file change causes the test failure. Sorry about that!

[akhuang]

Ok, I think the actual object file diff comes from this file: https://github.com/chromium/chromium/blob/master/sandbox/linux/syscall_broker/broker_command.cc
I checked that the test fails if I only build this object file with this change.

pasted the diff here: https://reviews.llvm.org/P8229

I also uploaded another build repro on the chromium bug.

[dmgreen]

OK, Thanks. I can see how that one would be wrong. Thanks for the example! We'll see what we can do.

[NickGuy]

Looks like we've lost access to https://crbug.com/1114852, so can't access the build repro. Is this something you can sort out for us?

[akhuang]

Oh no, I'll upload the file here since I'm not sure how to make the bug visible again: https://reviews.llvm.org/F12619177

clang -cc1 -triple thumbv7-unknown-linux-android16 -emit-obj --mrelax-relocations -mnoexecstack -disable-free -mrelocation-model pic -pic-level 2 -fmerge-all-constants -mframe-pointer=none -relaxed-aliasing -fno-rounding-math -mconstructor-aliases -munwind-tables -target-cpu generic -target-feature +soft-float-abi -target-feature +vfp2 -target-feature +vfp2sp -target-feature +vfp3 -target-feature +vfp3d16 -target-feature +vfp3d16sp -target-feature +vfp3sp -target-feature -fp16 -target-feature -vfp4 -target-feature -vfp4d16 -target-feature -vfp4d16sp -target-feature -vfp4sp -target-feature -fp-armv8 -target-feature -fp-armv8d16 -target-feature -fp-armv8d16sp -target-feature -fp-armv8sp -target-feature -fullfp16 -target-feature +fp64 -target-feature +d32 -target-feature +neon -target-feature -crypto -target-feature -fp16fml -target-abi aapcs-linux -mfloat-abi soft -fallow-half-arguments-and-returns -fno-split-dwarf-inlining -debug-info-kind=line-tables-only -dwarf-version=3 -debugger-tuning=gdb -ffunction-sections -fdata-sections -nostdinc++ -Oz -std=c++14 -fdeprecated-macro -fvisibility hidden -stack-protector 1 -stack-protector-buffer-size 4 -fno-rtti -fno-signed-char -fgnuc-version=4.2.1 -vectorize-slp -mllvm -instcombine-lower-dbg-declare=0 -x c++ repro-broker-command.cc

[akhuang]

This comment was removed by akhuang.

[thakis]

(I made the crbug public again)

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp

 }
 
 bool ARMBaseInstrInfo::DefinesPredicate(
     MachineInstr &MI, std::vector<MachineOperand> &Pred) const {
   bool Found = false;
   for (unsigned i = 0, e = MI.getNumOperands(); i != e; ++i) {
     const MachineOperand &MO = MI.getOperand(i);
     if ((MO.isRegMask() && MO.clobbersPhysReg(ARM::CPSR)) ||
         (MO.isReg() && MO.isDef() && MO.getReg() == ARM::CPSR)) {
+
+      // Filter out T1 instructions that have a dead CPSR,
+      // allowing IT blocks to be generated containing T1 instructions
+      const MCInstrDesc &MCID = MI.getDesc();
+      if(MCID.TSFlags & ARMII::ThumbArithFlagSetting && MO.isDead()) {
+        continue;
+      }
+
       Pred.push_back(MO);
       Found = true;
     }
   }
 
   return Found;
 }
 

llvm/test/CodeGen/Thumb2/constant-hoisting.ll

 ; CHECK-V6M-NEXT:    .p2align 2
 ; CHECK-V6M-NEXT:  .LCPI0_0:
 ; CHECK-V6M-NEXT:    .long 537923600
 ;
 ; CHECK-V7M-LABEL: test_values:
 ; CHECK-V7M:         mov r2, r0
 ; CHECK-V7M-NEXT:    ldr r0, .LCPI0_0
 ; CHECK-V7M-NEXT:    cmp r2, #50
-; CHECK-V7M-NEXT:    beq .LBB0_5
+; CHECK-V7M-NEXT:    beq .LBB0_3
 ; CHECK-V7M-NEXT:    cmp r2, #1
-; CHECK-V7M-NEXT:    beq .LBB0_7
+; CHECK-V7M-NEXT:    ittt eq
+; CHECK-V7M-NEXT:    addeq r0, r1
+; CHECK-V7M-NEXT:    addeq r0, #1
+; CHECK-V7M-NEXT:    bxeq lr
 ; CHECK-V7M-NEXT:    cmp r2, #30
-; CHECK-V7M-NEXT:    beq .LBB0_8
-; CHECK-V7M-NEXT:    cbnz r2, .LBB0_6
+; CHECK-V7M-NEXT:    ittt eq
+; CHECK-V7M-NEXT:    addeq r0, r1
+; CHECK-V7M-NEXT:    addeq r0, #2
+; CHECK-V7M-NEXT:    bxeq lr
+; CHECK-V7M-NEXT:    cbnz r2, .LBB0_4
+; CHECK-V7M-NEXT:  .LBB0_2:
 ; CHECK-V7M-NEXT:    add r0, r1
 ; CHECK-V7M-NEXT:    bx lr
-; CHECK-V7M-NEXT:  .LBB0_5:
+; CHECK-V7M-NEXT:  .LBB0_3:
 ; CHECK-V7M-NEXT:    add r0, r1
 ; CHECK-V7M-NEXT:    adds r0, #4
-; CHECK-V7M-NEXT:  .LBB0_6:
-; CHECK-V7M-NEXT:    bx lr
-; CHECK-V7M-NEXT:  .LBB0_7:
-; CHECK-V7M-NEXT:    add r0, r1
-; CHECK-V7M-NEXT:    adds r0, #1
-; CHECK-V7M-NEXT:    bx lr
-; CHECK-V7M-NEXT:  .LBB0_8:
-; CHECK-V7M-NEXT:    add r0, r1
-; CHECK-V7M-NEXT:    adds r0, #2
+; CHECK-V7M-NEXT:  .LBB0_4:
 ; CHECK-V7M-NEXT:    bx lr
 ; CHECK-V7M-NEXT:    .p2align 2
 ; CHECK-V7M-NEXT:  .LCPI0_0:
 ; CHECK-V7M-NEXT:    .long 537923600
 entry:
   switch i32 %a, label %return [
     i32 0, label %sw.bb
     i32 1, label %sw.bb1