Page MenuHomePhabricator

[DwarfExpression] Refactor dwarf expression (NFC)
ClosedPublic

Authored by djtodoro on May 16 2019, 4:21 AM.

Details

Summary

Refactor location description kind in order to be easier for extensions (needed for D60866).
In addition, cut off some bits from the other class fields.

Diff Detail

Repository
rL LLVM

Event Timeline

djtodoro created this revision.May 16 2019, 4:21 AM
aprantl added inline comments.May 16 2019, 3:11 PM
lib/CodeGen/AsmPrinter/DwarfExpression.h
124 ↗(On Diff #199785)

Perhaps I'm forgetting about a use-case here, but shouldn't memory and register be mutually exclusive? If so, should they share the same bit?

djtodoro updated this revision to Diff 200020.May 17 2019, 4:38 AM

-For now, all expression kinds are mutually exclusive, therefore handle it properly.

djtodoro marked an inline comment as done.May 17 2019, 4:41 AM
djtodoro added inline comments.
lib/CodeGen/AsmPrinter/DwarfExpression.h
124 ↗(On Diff #199785)

Yes, thanks! We will need additional flags in order to mark parameter and entry value expressions (e.g. at the same time an expression will be entry value and register).

aprantl added inline comments.May 17 2019, 9:33 AM
lib/CodeGen/AsmPrinter/DwarfExpression.h
137 ↗(On Diff #200020)

We can let the compiler do this for us by defining:

unsigned LocationKind : 3;
unsigned LocationFlags: ...

and initializing it to zero in the constructor. If we are worried about memory use here, I'd also cut off some bits from DwarfVersion.

djtodoro marked an inline comment as done.May 20 2019, 6:07 AM
djtodoro added inline comments.
lib/CodeGen/AsmPrinter/DwarfExpression.h
137 ↗(On Diff #200020)

Good idea, thanks!

djtodoro updated this revision to Diff 200263.May 20 2019, 6:11 AM

-Introduce the LokcationKind and the LokcationFlags
-Cut off bits from the DwarfVersion

dstenb added a subscriber: dstenb.May 20 2019, 6:27 AM
dstenb added inline comments.
lib/CodeGen/AsmPrinter/DwarfExpression.h
114 ↗(On Diff #200263)

I don't think this has any effect on x86 for example, since SubRegisterSizeInBits will be padded to 32 bits. Perhaps it is a non-intrusive change to move DwarfVersion down to the other bit-fields?

djtodoro marked an inline comment as done.May 20 2019, 6:36 AM
djtodoro added inline comments.
lib/CodeGen/AsmPrinter/DwarfExpression.h
114 ↗(On Diff #200263)

I see.. I didn't think about the alignment.
I think we can let that as unsigned.

djtodoro updated this revision to Diff 200273.May 20 2019, 6:59 AM

-Keep the DwarfVersion as unsigned

aprantl added inline comments.May 20 2019, 9:55 AM
lib/CodeGen/AsmPrinter/DwarfExpression.h
114 ↗(On Diff #200263)

I think what @dstenb meant was to make this into:

uint64_t OffsetInBits = 0;
 unsigned SubRegisterOffsetInBits = 0;

 /// The kind of location description being produced.
 enum { Unknown = 0, Register, Memory, Implicit };

 unsigned LocationKind : 3;
 unsigned LocationFlags : 2; 
 unsigned DwarfVersion : 4;

this way the packed bitfields all share one 32-bit slot.

aprantl added inline comments.May 20 2019, 9:56 AM
lib/CodeGen/AsmPrinter/DwarfExpression.h
118 ↗(On Diff #200273)

and these could be unsigned : 16.

djtodoro marked an inline comment as done.May 20 2019, 11:26 AM
djtodoro added inline comments.
lib/CodeGen/AsmPrinter/DwarfExpression.h
114 ↗(On Diff #200263)

Sure. I understood the idea, but I didn’t want to move (and refactor) those other fields in this commit, but refactoring it this way makes sense to me. :)

I will do that way.

djtodoro updated this revision to Diff 200409.May 20 2019, 11:49 PM
djtodoro retitled this revision from [DwarfExpression] Refactor dwarf expression kind (NFC) to [DwarfExpression] Refactor dwarf expression (NFC).
djtodoro edited the summary of this revision. (Show Details)

-Cut off some bits from the other class fields
-Rename the title
-Change the summary

aprantl accepted this revision.May 21 2019, 11:08 AM

Thanks! One more nit inside, otherwise good to go.

lib/CodeGen/AsmPrinter/DwarfExpression.h
146 ↗(On Diff #200409)

assert(SizeInBits < 65536 && OffsetInBits < 65536);

This revision is now accepted and ready to land.May 21 2019, 11:08 AM
djtodoro updated this revision to Diff 200648.May 21 2019, 11:24 PM

-Add an assertion when emitting pieces info about a subregister

@aprantl Thanks a lot for the review!

aprantl accepted this revision.May 22 2019, 8:45 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 23 2019, 3:35 AM