Page MenuHomePhabricator

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

Authored by NickGuy on Mon, Jul 13, 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.Mon, Jul 13, 3:36 AM
dmgreen added inline comments.Wed, Jul 15, 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.Mon, Aug 3, 1:17 AM

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

dmgreen accepted this revision.Mon, Aug 3, 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.Mon, Aug 3, 2:13 AM
This revision was landed with ongoing or failed builds.Mon, Aug 3, 5:20 AM
This revision was automatically updated to reflect the committed changes.