This is an archive of the discontinued LLVM Phabricator instance.

Fix removal of dead elements from PressureDiff
ClosedPublic

Authored by ygribov on Aug 30 2018, 10:01 AM.

Diff Detail

Event Timeline

ygribov created this revision.Aug 30 2018, 10:01 AM
MatzeB accepted this revision.Aug 30 2018, 4:34 PM

Good catch, the fix looks good to me. I assume you built llvm will all (non-experimental) targets and tested them?

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...

This revision is now accepted and ready to land.Aug 30 2018, 4:34 PM

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.

This revision was automatically updated to reflect the committed changes.
ygribov reopened this revision.Sep 18 2018, 7:50 AM

Reopening (commit caused lit regressions).

This revision is now accepted and ready to land.Sep 18 2018, 7:50 AM
ygribov updated this revision to Diff 167094.Sep 26 2018, 3:42 AM

Meanwhile few more tests required changes.

This revision was automatically updated to reflect the committed changes.