This is an archive of the discontinued LLVM Phabricator instance.

[NFC] Use ADT/Bitfields in Instructions
ClosedPublic

Authored by gchatelet on Jun 11 2020, 7:56 AM.

Details

Summary

This is an example patch for D81580.

Diff Detail

Event Timeline

gchatelet created this revision.Jun 11 2020, 7:56 AM
gchatelet marked 10 inline comments as done.Jun 11 2020, 8:23 AM
gchatelet added inline comments.
llvm/include/llvm/IR/InstrTypes.h
1099

Previous code was not explicit about the size of the packed bit, so it was possible to pass in values that would be truncated when stored (values >=8192)
Additionally the first two bits were left unused which would force a shift operation for both get and set.

I believe we can just make the bitfield start at 0

llvm/include/llvm/IR/Instructions.h
429

The first bit is unused, this leads to an additional shift operation for both get and set.
Also the field was not bounded, making it impossible to know which bits are available.

534

The bit in position 1 is unused?

569

The masking pattern here is dubious.

582

The masking pattern here is dubious.

734

Same here the bit in position 1 is unused.

748

The Operation field is not bounded making it impossible to know which next bit is usable.

llvm/lib/IR/Instructions.cpp
1401

Here only the lower bits are masked so theoretically encode(Align) could occupy more than 6 bits and start overrun the next field storing AtomicOrdering

1480

ditto.

craig.topper added inline comments.
llvm/include/llvm/IR/InstrTypes.h
1099

Isn't TailCallKind from CallInst in bits 0 and 1?

craig.topper added inline comments.Jun 11 2020, 2:21 PM
llvm/include/llvm/IR/Instructions.h
534

It used to be syncscope before that was extended to support arbitrary scopes instead of just single thread and system

734

Same as above

I really like the extra abstraction layer! Some reviews inline.

llvm/include/llvm/ADT/Bitfields.h
75 ↗(On Diff #270144)

Shouldn't this be (8 * sizeof(type)) instead of 64?

76 ↗(On Diff #270144)

if sizeof(1ULL) == 64 and size == 64 then you're doomed. what about ~static_cast<type>(0) >> (8 * sizeof(type) - size)` ?

llvm/include/llvm/IR/InstrTypes.h
1361

Looks like this is breaking the abstraction. Make this a (static) method?

gchatelet updated this revision to Diff 271124.Jun 16 2020, 9:42 AM
gchatelet marked 2 inline comments as done.
  • Handle bounds and signed integer, redesign the implementation
gchatelet marked 8 inline comments as done.Jun 16 2020, 9:58 AM
gchatelet added inline comments.
llvm/include/llvm/ADT/Bitfields.h
76 ↗(On Diff #270144)

Thx for your comment, I had to be more careful about signed-ness but I believe the new implementation is much better,

llvm/include/llvm/IR/InstrTypes.h
1099

You're right, thx for pointing this out it was not obvious.

1361

The abstraction will now check bounds for enums.

llvm/include/llvm/IR/Instructions.h
534

Thank you, I've updated the patch to reflect this knowledge.

gchatelet updated this revision to Diff 271219.Jun 16 2020, 3:02 PM
gchatelet marked 3 inline comments as done.
  • Update style.
gchatelet updated this revision to Diff 271294.Jun 17 2020, 1:30 AM
  • Modify all enums to be unsigned
gchatelet updated this revision to Diff 271618.Jun 18 2020, 2:25 AM

Fix style and rebase

gchatelet updated this revision to Diff 272458.Jun 22 2020, 8:48 AM

Fix style.

nhaehnle added inline comments.Jun 23 2020, 5:03 AM
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
4556

This change and the one below don't look wrong, but they seem unrelated to what this patch is claiming to achieve.

gchatelet marked 2 inline comments as done.Jun 23 2020, 7:25 AM
gchatelet added inline comments.
llvm/lib/Target/AMDGPU/SIISelLowering.cpp
4556

They seem unrelated but they are : )
This has to do with ICmpInst::Predicate now being explicitly unsigned for efficiency reasons.
The comparison below in the if statement would then trigger a warning because we compare signed and unsigned types.
Changing Condcode to unsigned makes sure the compiler is happy.

gchatelet updated this revision to Diff 272719.Jun 23 2020, 7:28 AM
gchatelet marked an inline comment as done.
  • Fix misaligned documentation

Can you add a comment that explains when and why to use this over actual C(++) bitfields?

llvm/include/llvm/ADT/Bitfields.h
25 ↗(On Diff #272719)

Does this doxygen comment just hang in open space? It would be unfortunate if it didn't make it into the documentation just because it's not attached to a syntax node.

gchatelet marked an inline comment as done.Jun 26 2020, 8:04 AM

Can you add a comment that explains when and why to use this over actual C(++) bitfields?

Last version of the design is here https://reviews.llvm.org/D82454#change-SB8sVbudjRwc
I've updated the documentation. WDYT?

gchatelet updated this revision to Diff 274083.Jun 29 2020, 6:09 AM
  • rebase with the new API
gchatelet updated this revision to Diff 274088.Jun 29 2020, 6:35 AM

Use the optimized test method

I'll submit this if nobody objects

This revision was not accepted when it landed; it landed in state Needs Review.Jul 3 2020, 12:29 AM
This revision was automatically updated to reflect the committed changes.