[PATCH] D51495: Fix removal of dead elements from PressureDiff

[Yury Gribov via Phabricator via llvm-commits]

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