Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

remove Aligned from SubWord #170

Merged
merged 1 commit into from
Oct 13, 2021
Merged

remove Aligned from SubWord #170

merged 1 commit into from
Oct 13, 2021

Conversation

Ptival
Copy link
Contributor

@Ptival Ptival commented Oct 6, 2021

The Aligned constructor to SubWord is morally equivalent to SubWord 0 0.
While it is nice as a visual marker in code, it makes consuming SubWords
often more verbose/redundant than it needs be.

This proposes removing it altogether, while preserving some readability with
the aligned value. We could also add an Aligned pattern for SubWord 0 _,
but at the moment, we never actually need to pattern-match on Aligned.

It also makes it so splitWord and getBitStringPartial can take a SubWord
as input, rather than the length and word separately, which looked a bit out of
place.

@Ptival Ptival requested a review from atomb October 6, 2021 18:56
atomb
atomb approved these changes Oct 12, 2021
View reviewed changes
Copy link
Contributor

@atomb atomb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a nice simplification. I wonder whether making SubWord a newtype could perhaps improve performance (but I'm not sure if it would).

src/Data/LLVM/BitCode/GetBits.hs Show resolved Hide resolved
@Ptival
Copy link
Contributor Author

Ptival commented Oct 12, 2021

I gave it a try and it did not seem to make much of a difference.

Not sure it's great given me need to pair the two arguments together, and newtype prevents us from making them strict too.

@atomb
Copy link
Contributor

atomb commented Oct 12, 2021

Ah, I was thinking of a newtype in record form with strict fields. But it may not actually be an improvement.

@Ptival
Copy link
Contributor Author

Ptival commented Oct 13, 2021

It seems like newtypes can neither have multiple fields, nor have strict fields, unless I don't know about some trick/extension.

@atomb
Copy link
Contributor

atomb commented Oct 13, 2021

Oh, you're right. I was misremembering the record syntax. Although you can use that syntax, it only allows one field.

@Ptival
remove Aligned from SubWord
b1616a2 
The `Aligned` constructor to `SubWord` is morally equivalent to `SubWord 0 0`.
While it is nice as a visual marker in code, it makes consuming `SubWord`s
often more verbose/redundant than it needs be.

This proposes removing it altogether, while preserving some readability with
the `aligned` value.  We could also add an `Aligned` pattern for `SubWord 0 _`,
but at the moment, we never actually need to pattern-match on `Aligned`.

It also makes it so `splitWord` and `getBitStringPartial` can take a `SubWord`
as input, rather than the length and word separately, which looked a bit out of
place.
@Ptival Ptival merged commit 4b2e006 into master Oct 13, 2021
@Ptival Ptival deleted the vr/subword-without-aligned branch October 13, 2021 19:18
RyanGlScott added a commit to GaloisInc/crucible that referenced this pull request Dec 5, 2021
@RyanGlScott
Bump llvm-pretty{-bc-parser} submodules, adapt to ValPoison
bad01d0 
This bumps the `llvm-pretty` submodule to bring in the following PRs:

* GaloisInc/llvm-pretty#86 (support LLVM 11 `IsDefault` fields in metadata)
* GaloisInc/llvm-pretty#87 (Relax the Symbol requirement to `ConstBlockAddr`)
* GaloisInc/llvm-pretty#88 (add LLVM12 poison value)

This also bumps the `llvm-pretty-bc-parser` submodule to bring in the
following PRs:

* GaloisInc/llvm-pretty-bc-parser#171 (don't consume fast math flag entry)
* GaloisInc/llvm-pretty-bc-parser#169 (make `getBitString` assumptions explicit)
* GaloisInc/llvm-pretty-bc-parser#168 (support for LLVM 11 `IsDefault` fields in metadata)
* GaloisInc/llvm-pretty-bc-parser#170 (remove `Aligned` from `SubWord`)
* GaloisInc/llvm-pretty-bc-parser#172 (fix BLOCKADDRESS parsing)
* GaloisInc/llvm-pretty-bc-parser#173 (parse LLVM12 poison value)
* GaloisInc/llvm-pretty-bc-parser#178 (use Seq over list in PartialModule)

Of these, the only PR that requires code changes in Crucible is
GaloisInc/llvm-pretty#88 (add LLVM12 poison value), as this adds a new
`ValPoison` constructor to `Value'`. Properly handling LLVM poison is a large
task that I do not wish to tackle here (see #366 for discussion of how one
might do this). For now, this PR does the bare minimum:

* In `Lang.Crucible.LLVM.Translation.BlockInfo.useVal`, treat `ValPoison`
  as not referencing any identifiers.
* The other two potential sites where `ValPoison` could be matched on are in
  `Lang.Crucible.LLVM.Translation.Constant.transConstant'` and
  `Lang.Crucible.LLVM.Translation.Expr.transValue`. Since we are not handling
  `ValPoison` for now, both of these functions will simply error if they
  encounter `ValPoison`.
* I have added a section to `crucible-llvm`'s `limitations.md` document which
  describes the above limitations of poison handling.
RyanGlScott added a commit to GaloisInc/crucible that referenced this pull request Dec 6, 2021
@RyanGlScott
Bump llvm-pretty{-bc-parser} submodules, adapt to ValPoison
1796854 
This bumps the `llvm-pretty` submodule to bring in the following PRs:

* GaloisInc/llvm-pretty#86 (support LLVM 11 `IsDefault` fields in metadata)
* GaloisInc/llvm-pretty#87 (Relax the Symbol requirement to `ConstBlockAddr`)
* GaloisInc/llvm-pretty#88 (add LLVM12 poison value)
* GaloisInc/llvm-pretty#90 (Support structs with bitfields in `Text.LLVM.DebugUtils`)

This also bumps the `llvm-pretty-bc-parser` submodule to bring in the
following PRs:

* GaloisInc/llvm-pretty-bc-parser#171 (don't consume fast math flag entry)
* GaloisInc/llvm-pretty-bc-parser#169 (make `getBitString` assumptions explicit)
* GaloisInc/llvm-pretty-bc-parser#168 (support for LLVM 11 `IsDefault` fields in metadata)
* GaloisInc/llvm-pretty-bc-parser#170 (remove `Aligned` from `SubWord`)
* GaloisInc/llvm-pretty-bc-parser#172 (fix BLOCKADDRESS parsing)
* GaloisInc/llvm-pretty-bc-parser#173 (parse LLVM12 poison value)
* GaloisInc/llvm-pretty-bc-parser#178 (use Seq over list in PartialModule)

Of these, the only PR that requires code changes in Crucible is
GaloisInc/llvm-pretty#88 (add LLVM12 poison value), as this adds a new
`ValPoison` constructor to `Value'`. Properly handling LLVM poison is a large
task that I do not wish to tackle here (see #366 for discussion of how one
might do this). For now, this PR does the bare minimum:

* In `Lang.Crucible.LLVM.Translation.BlockInfo.useVal`, treat `ValPoison`
  as not referencing any identifiers.
* The other two potential sites where `ValPoison` could be matched on are in
  `Lang.Crucible.LLVM.Translation.Constant.transConstant'` and
  `Lang.Crucible.LLVM.Translation.Expr.transValue`. Since we are not handling
  `ValPoison` for now, both of these functions will simply error if they
  encounter `ValPoison`.
* I have added a section to `crucible-llvm`'s `limitations.md` document which
  describes the above limitations of poison handling.
@RyanGlScott RyanGlScott mentioned this pull request Dec 6, 2021
Merged
RyanGlScott added a commit to GaloisInc/crucible that referenced this pull request Dec 6, 2021
@RyanGlScott
Bump llvm-pretty{-bc-parser} submodules, adapt to ValPoison (#936)
acd37d5 
This bumps the `llvm-pretty` submodule to bring in the following PRs:

* GaloisInc/llvm-pretty#86 (support LLVM 11 `IsDefault` fields in metadata)
* GaloisInc/llvm-pretty#87 (Relax the Symbol requirement to `ConstBlockAddr`)
* GaloisInc/llvm-pretty#88 (add LLVM12 poison value)
* GaloisInc/llvm-pretty#90 (Support structs with bitfields in `Text.LLVM.DebugUtils`)

This also bumps the `llvm-pretty-bc-parser` submodule to bring in the
following PRs:

* GaloisInc/llvm-pretty-bc-parser#171 (don't consume fast math flag entry)
* GaloisInc/llvm-pretty-bc-parser#169 (make `getBitString` assumptions explicit)
* GaloisInc/llvm-pretty-bc-parser#168 (support for LLVM 11 `IsDefault` fields in metadata)
* GaloisInc/llvm-pretty-bc-parser#170 (remove `Aligned` from `SubWord`)
* GaloisInc/llvm-pretty-bc-parser#172 (fix BLOCKADDRESS parsing)
* GaloisInc/llvm-pretty-bc-parser#173 (parse LLVM12 poison value)
* GaloisInc/llvm-pretty-bc-parser#178 (use Seq over list in PartialModule)

Of these, the only PR that requires code changes in Crucible is
GaloisInc/llvm-pretty#88 (add LLVM12 poison value), as this adds a new
`ValPoison` constructor to `Value'`. Properly handling LLVM poison is a large
task that I do not wish to tackle here (see #366 for discussion of how one
might do this). For now, this PR does the bare minimum:

* In `Lang.Crucible.LLVM.Translation.BlockInfo.useVal`, treat `ValPoison`
  as not referencing any identifiers.
* The other two potential sites where `ValPoison` could be matched on are in
  `Lang.Crucible.LLVM.Translation.Constant.transConstant'` and
  `Lang.Crucible.LLVM.Translation.Expr.transValue`. Since we are not handling
  `ValPoison` for now, both of these functions will simply error if they
  encounter `ValPoison`.
* I have added a section to `crucible-llvm`'s `limitations.md` document which
  describes the above limitations of poison handling.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atomb atomb atomb approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Ptival @atomb