This is an archive of the discontinued LLVM Phabricator instance.

IR: New representation for CFI and virtual call optimization pass metadata.
ClosedPublic

Authored by pcc on Jun 6 2016, 8:01 PM.

Details

Summary

The bitset metadata currently used in LLVM has a few problems:

  1. It has the wrong name. The name "bitset" refers to an implementation detail of one use of the metadata (i.e. its original use case, CFI). This makes it harder to understand, as the name makes no sense in the context of virtual call optimization.
  1. It is represented using a global named metadata node, rather than being directly associated with a global. This makes it harder to manipulate the metadata when rebuilding global variables, summarise it as part of ThinLTO and drop unused metadata when associated globals are dropped. For this reason, CFI does not currently work correctly when both CFI and vcall opt are enabled, as vcall opt needs to rebuild vtable globals, and fails to associate metadata with the rebuilt globals. As I understand it, the same problem could also affect ASan, which rebuilds globals with a red zone.

This patch solves both of those problems in the following way:

  1. Rename the metadata to "type metadata". This new name reflects how the metadata is currently being used (i.e. to represent type information for CFI and vtable opt). The new name is reflected in the name for the associated intrinsic (llvm.type.test) and pass (LowerTypeTests).
  1. Attach metadata directly to the globals that it pertains to, rather than using the "llvm.bitsets" global metadata node as we are doing now. This is done using the newly introduced capability to attach metadata to global variables (r271348 and r271358).

See also: http://lists.llvm.org/pipermail/llvm-dev/2016-June/100462.html

Depends on D21052

Diff Detail

Repository
rL LLVM

Event Timeline

pcc updated this revision to Diff 59820.Jun 6 2016, 8:01 PM
pcc retitled this revision from to IR: New representation for CFI and virtual call optimization pass metadata..
pcc updated this object.
pcc added reviewers: mehdi_amini, kcc.
pcc added a subscriber: llvm-commits.
pcc added a subscriber: eugenis.Jun 7 2016, 11:47 AM
eugenis added inline comments.Jun 9 2016, 1:34 PM
lib/Transforms/IPO/CrossDSOCFI.cpp
52 ↗(On Diff #59820)

rename to extractTypeMetadataTypeId?

87 ↗(On Diff #59820)

Maybe move this check to the caller? This function does not really use the GlobalObject argument.

98 ↗(On Diff #59820)

There are still some references to "BitSet" in this file. Is it intentional?
I understand the devirt thing is using BitSet to refer to the bits stored near the virtual table, which is a completely separate concept from the type test metadata.

lib/Transforms/IPO/LowerTypeTests.cpp
832 ↗(On Diff #59820)

This comment is now confusing, there is no "Op".

906 ↗(On Diff #59820)

"will be used later"

960 ↗(On Diff #59820)

Would you like to factor out this code that applies a function to all functions and globals in a module?

pcc updated this revision to Diff 60629.Jun 13 2016, 5:00 PM
pcc marked 4 inline comments as done.
  • Address review comments
lib/Transforms/IPO/CrossDSOCFI.cpp
52 ↗(On Diff #59820)

Done (to extractNumericTypeId).

98 ↗(On Diff #59820)

No. I spotted a few more missed references in LowerTypeTests, also fixed.

Yes, the references in devirt are correct.

lib/Transforms/IPO/LowerTypeTests.cpp
960 ↗(On Diff #59820)

I considered it, but I'm not sure whether that or a Module::global_objects() range would be better.

davide added a subscriber: davide.Jun 14 2016, 4:21 PM
eugenis added inline comments.Jun 21 2016, 4:57 PM
lib/Transforms/IPO/CrossDSOCFI.cpp
77 ↗(On Diff #60629)

GlobalObject argument is now unused.

lib/Transforms/IPO/LowerTypeTests.cpp
960 ↗(On Diff #60629)

I'm surprised we don't have Module::global_objects() yet. It sounds like a great idea.

pcc marked an inline comment as done.Jun 21 2016, 6:31 PM
pcc added inline comments.
lib/Transforms/IPO/CrossDSOCFI.cpp
77 ↗(On Diff #60629)

Removed

lib/Transforms/IPO/LowerTypeTests.cpp
960 ↗(On Diff #60629)

Done in D21580; this patch now depends on it.

pcc updated this revision to Diff 61483.Jun 21 2016, 6:31 PM
pcc marked an inline comment as done.
  • Refresh
  • Address review comments
pcc updated this revision to Diff 61619.Jun 22 2016, 3:28 PM

Refresh, depend on D21624

pcc added a comment.Jun 24 2016, 11:39 AM

@eugenis FYI, this is unblocked now.

pcc edited reviewers, added: eugenis; removed: kcc, mehdi_amini.Jun 24 2016, 11:40 AM
pcc removed a subscriber: eugenis.
eugenis accepted this revision.Jun 24 2016, 12:45 PM
eugenis edited edge metadata.

LGTM
Next time please do the renaming in a separate change - it makes the diff very hard to read.

This revision is now accepted and ready to land.Jun 24 2016, 12:45 PM
This revision was automatically updated to reflect the committed changes.
llvm/trunk/test/tools/gold/X86/opt-level.ll