[PATCH] D51495: Fix removal of dead elements from PressureDiff
Yury Gribov via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Fri Aug 31 01:37:39 PDT 2018
ygribov added a comment.
> Good catch, the fix looks good to me. I assume you built llvm will all (non-experimental) targets and tested them?
Yes, I ran cmake with default settings so all default cores were built. One caveat is that testing was done on Windows (~27K tests, done according to instructions in https://llvm.org/docs/GettingStartedVS.html).
> Independently of this patch it is worrying that this requires test changes. That means X86 schedules produce intermediate pressure sets with MaxPSets/16 different pressure sets inside...
Yes, here's what I'm getting for e.g. divrem8_ext.ll:
- PressureChanges 0x0000000002663400 {{PSetID=2 UnitInc=1 }, {PSetID=11 UnitInc=1 }, {PSetID=12 UnitInc=1 }, {PSetID=13 ...}, ...} llvm::PressureChange[16]
+ [0] {PSetID=2 UnitInc=1 } llvm::PressureChange GR8_ABCD_L
+ [1] {PSetID=11 UnitInc=1 } llvm::PressureChange GR32_TC
+ [2] {PSetID=12 UnitInc=1 } llvm::PressureChange LOW32_ADDR_ACCESS_with_sub_32bit+GR64_NOREX_and_GR64_TCW64
+ [3] {PSetID=13 UnitInc=1 } llvm::PressureChange GR64_NOREX_and_GR64_TC
+ [4] {PSetID=14 UnitInc=1 } llvm::PressureChange LOW32_ADDR_ACCESS_with_sub_32bit+GR64_NOREX_and_GR64_TC
+ [5] {PSetID=18 UnitInc=1 } llvm::PressureChange GR64_NOREX
+ [6] {PSetID=19 UnitInc=1 } llvm::PressureChange GR64_TCW64
+ [7] {PSetID=20 UnitInc=1 } llvm::PressureChange LOW32_ADDR_ACCESS_with_sub_32bit+GR64_TCW64
+ [8] {PSetID=21 UnitInc=1 } llvm::PressureChange GR64_TC
+ [9] {PSetID=22 UnitInc=1 } llvm::PressureChange LOW32_ADDR_ACCESS_with_sub_32bit+GR64_TC
+ [10] {PSetID=23 UnitInc=1 } llvm::PressureChange GR64_TC+GR64_TCW64
+ [11] {PSetID=25 UnitInc=-1 } llvm::PressureChange GR8+GR64_NOREX
+ [12] {PSetID=26 UnitInc=-1 } llvm::PressureChange GR8+GR64_TCW64
+ [13] {PSetID=28 UnitInc=-1 } llvm::PressureChange GR8+GR64_TC
+ [14] {PSetID=30 UnitInc=-1 } llvm::PressureChange GR16
+ [15] {PSetID=30 UnitInc=-1 } llvm::PressureChange GR16
As a side note the current limit of 16 elements may be too strict for some targets (e.g. it caused a lot of noise on our non-orthogonal architecture until I increased it significantly) so it may make sense to switch to dynamic array.
Repository:
rL LLVM
https://reviews.llvm.org/D51495
More information about the llvm-commits
mailing list