Page MenuHomePhabricator

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

Authored by NickGuy on Jul 13 2020, 3:36 AM.

Details

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

ProgramPre-patchPost-patchdiff
test-suite.../Prolangs-C++/ocean/ocean.test21682160-0.4%
test-suite...s/Misc/richards_benchmark.test14321428-0.3%
test-suite...out-C++/Shootout-C++-hash.test16961692-0.2%
test-suite...ut-C++/Shootout-C++-hash2.test19241920-0.2%
test-suite.../Applications/spiff/spiff.test1220012176-0.2%
test-suite.../Benchmarks/Ptrdist/ks/ks.test27682764-0.1%
test-suite...Source/Benchmarks/sim/sim.test87128704-0.1%
test-suite...s/MallocBench/cfrac/cfrac.test1236012352-0.1%
test-suite...lFlow-dbl/ControlFlow-dbl.test1289612888-0.1%
test-suite...chmarks/MallocBench/gs/gs.test7816078112-0.1%
test-suite...lications/sqlite3/sqlite3.test168436168340-0.1%
test-suite...telecomm-gsm/telecomm-gsm.test1501215004-0.1%
test-suite...ediabench/gsm/toast/toast.test1501215004-0.1%
test-suite...urce/Applications/hbd/hbd.test1850418496-0.0%
test-suite...encode/alacconvert-encode.test1992419916-0.0%
test-suite...decode/alacconvert-decode.test1992419916-0.0%
test-suite.../Benchmarks/Ptrdist/bc/bc.test2075220744-0.0%
test-suite...BLambdaLoops/lcalsBLambda.test8576485732-0.0%
test-suite...pplications/oggenc/oggenc.test9017690144-0.0%
test-suite...SubsetCRawLoops/lcalsCRaw.test9053290500-0.0%
test-suite...CLambdaLoops/lcalsCLambda.test9062890596-0.0%
test-suite...ks/Prolangs-C/agrep/agrep.test2388423876-0.0%
test-suite...lFlow-flt/ControlFlow-flt.test1338413380-0.0%
test-suite...lications/ClamAV/clamscan.test245996245924-0.0%
test-suite...terchange/LoopInterchange.test5912859112-0.0%
test-suite...eProcessing/Dilate/Dilate.test6037660360-0.0%
test-suite...Filtering/BilateralFilter.test6063260616-0.0%
test-suite...sion/AnisotropicDiffusion.test6068060664-0.0%
test-suite...ImageProcessing/Blur/blur.test6101661000-0.0%
test-suite...eProcessing/Dither/Dither.test6130461288-0.0%
test-suite...urce/Applications/lua/lua.test6152061504-0.0%
test-suite...yApps-C++/PENNANT/PENNANT.test3227632268-0.0%
test-suite...ProxyApps-C++/CLAMR/CLAMR.test217388217340-0.0%
test-suite...-typeset/consumer-typeset.test279140279084-0.0%
test-suite.../Applications/SPASS/SPASS.test201528201488-0.0%
test-suite...ications/JM/ldecod/ldecod.test128356128332-0.0%
test-suite...SubsetBRawLoops/lcalsBRaw.test8574885732-0.0%
test-suite...plications/d/make_dparser.test43956439640.0%
test-suite...SubsetARawLoops/lcalsARaw.test8853288516-0.0%
test-suite...ALambdaLoops/lcalsALambda.test8939689380-0.0%
test-suite...arks/mafft/pairlocalalign.test168656168632-0.0%
test-suite...nsumer-jpeg/consumer-jpeg.test6000059992-0.0%
test-suite...abench/jpeg/jpeg-6a/cjpeg.test6192061912-0.0%
test-suite...ce/Benchmarks/PAQ8p/paq8p.test4063640632-0.0%
test-suite...CI_Purple/SMG2000/smg2000.test89536895440.0%
test-suite...rks/tramp3d-v4/tramp3d-v4.test198420198404-0.0%
test-suite...ications/JM/lencod/lencod.test317976317952-0.0%
test-suite...Applications/kimwitu++/kc.test241500241492-0.0%
test-suite.../Benchmarks/Bullet/bullet.test296420296412-0.0%
test-suite...marks/7zip/7zip-benchmark.test3235803235840.0%
test-suite...peg2/mpeg2dec/mpeg2decode.test22968229760.0%
test-suite...cations/hexxagon/hexxagon.test722472280.1%
test-suite...s/Ptrdist/anagram/anagram.test194019440.2%
test-suite...Adobe-C++/stepanov_vector.test788879040.2%
test-suite...-C++/stepanov_abstraction.test634063560.3%
Geomean difference-0.0%
Pre-PatchPost-Patchdiff
count310.000000310.000000310.000000
mean19626.80000019624.077419-0.000062
std48727.31846148720.6648770.000428
min292.000000292.000000-0.003690
25%1090.0000001090.0000000.000000
50%2772.0000002770.0000000.000000
75%10185.00000010185.0000000.000000
max323580.000000323584.0000000.002524

Diff Detail

Event Timeline

NickGuy created this revision.Jul 13 2020, 3:36 AM
dmgreen added inline comments.Jul 15 2020, 1:06 AM
llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
589–596

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 updated this revision to Diff 282523.Aug 3 2020, 1:17 AM

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

dmgreen accepted this revision.Aug 3 2020, 2:13 AM

Thanks. LGTM with a very minor nitpick

llvm/lib/Target/ARM/ARMBaseInstrInfo.cpp
594

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

This revision is now accepted and ready to land.Aug 3 2020, 2:13 AM
This revision was landed with ongoing or failed builds.Aug 3 2020, 5:20 AM
This revision was automatically updated to reflect the committed changes.

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

Hi @akhuang, thanks for bringing this up.

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

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.

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 :)

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.

^

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.

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!

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.

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

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?

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

This comment was removed by akhuang.

(I made the crbug public again)