Skip to content

Comments

Add support for LLVM address spaces#280

Open
peterohanley wants to merge 6 commits intoGaloisInc:masterfrom
peterohanley:gus/upstreaming
Open

Add support for LLVM address spaces#280
peterohanley wants to merge 6 commits intoGaloisInc:masterfrom
peterohanley:gus/upstreaming

Conversation

@peterohanley
Copy link

Address spaces default to 0 and are currently mostly used by CHERI for capabilities (address space 200).

The thread_local property is properly read and reported.

Various formatting changes are implemented to more closely match llvm-dis output.

, decAttrs = []
, decComdat = protoComdat fp
-- TODO what if this isn't equal to adr?
, decAddrSpace = protoAddrSpace fp
Copy link
Author

Choose a reason for hiding this comment

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

I think this would still be a sensible construct if they were not equal, but it hasn't been used.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, that is tricky. Do you know what LLVM does here? If nothing else, it might be worth checking for an (opt-in) assert.

@peterohanley
Copy link
Author

The formatting changes to match llvm-dis break golden tests. They're meant for helping compare output, so I'll remove them for now and just leave the semantic changes.

, decAttrs = []
, decComdat = protoComdat fp
-- TODO what if this isn't equal to adr?
, decAddrSpace = protoAddrSpace fp
Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, that is tricky. Do you know what LLVM does here? If nothing else, it might be worth checking for an (opt-in) assert.

vis <- if length (recordFields r) > (6 + offset) && not (link `elem` [Internal, Private])
then field 6 visibility
else pure DefaultVisibility
tl <- if length (recordFields r) > (7 + offset) -- && not (link `elem` [Internal, Private])
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this comment be deleted? I think thread locality is parsed independently of the linkage, based on my reading of the LLVM implementation here.

Comment on lines +65 to +68
else case ptrty of
PtrTo as _ -> return as
PtrOpaque as -> return as
_ -> fail $ "Invalid type for value: " ++ show ptrty
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this case expression be simplified by defining it in terms of ptrAddrSpace from GaloisInc/llvm-pretty#148?

Comment on lines +185 to +188
if len == 0
then do
n <- show <$> nextSymbolId
return (Symbol n, 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you leave some comments explaining what is going on in the len == 0 case? From this commit, I gather that it related to unnamed symbols in some way, but the details aren't clear to me.

then
field 1 numeric
else
return 0
Copy link
Member

Choose a reason for hiding this comment

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

Another candidate for defaultAddrSpace, alhtough the AddrSpace constructor will need to move to the then.

setDataLayout :: DataLayout -> Parse ()
setDataLayout v = Parse $ do
ps <- get
put $! ps { psDataLayout = Just v }
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
put $! ps { psDataLayout = Just v }
setDataLayout v = Parse $ modify $ \ps -> ps { psDataLayout = Just v }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants