Const structs #476

pschachte · 2024-12-12T05:17:58Z

Generate LLVM constant structures in place of code to create structures all of whose contents are constants. This also applies to constant closures, so the code to optimise them in LLVM generation is now not needed, and to strings of length >1.

Replacing uses with ArgInt. This conversion happened anyway, later in the compiler, so not much is gained by having that PrimArg constructor. Huge number of changes to .exp files, but most are trivial changes to putchar calls.

Still generates bogus code in some cases.

Also recognise that constant structs are always not equal to 0

LLVM aligns structure fields, so we can't just use a single undef int the right size to fill in holes in a structure. Instead, we allocate the needed number of 1-byte undefs, plus the needed number of word-size undefs.

jimbxb

I will get back to the large files that I haven't read in some time. I also need to go through the changes in at least one of the larger exp files in more detail, but they seems sensible!

It's most unfortunate that there's a 100x in the mallocs in the drone test case. I suppose that's harder to get back now that the first drone is static and the compiler isn't smart enough to use a specialisation after that fact :/

jimbxb · 2024-12-12T07:28:23Z

test-cases/execution/nbody.exp

@@ -2,4 +2,4 @@ n: energy: -0.169075
 energy: -0.169088

 malloc count: (should be 5005 with multi-specz and 10006 without it)
-5005


Nice. A 0.1% efficiency improvement!

Can we update the comment above?

jimbxb · 2024-12-12T09:52:07Z

src/AST.hs

+    else do
+        gFlows <- getProcGlobalFlows pspec
+        globalFlowsUnions . (gFlows:) <$> mapM constGlobalFlows fields
+constsGlobalFlows fields = return emptyGlobalFlows -- XXX need to recurse!


Is it also possible there are global flows in the other fields? I think it's possible that there is another function pointer nested inside one of the other fields, and I think that should be considered.

jimbxb · 2024-12-12T09:55:53Z

test-cases/complex/exp/testcase_multi_specz-drone.exp

@@ -13,7 +13,7 @@
 ----------------------------------------------------------------------
 (0, 0, 0) #0
 (-1, -6, 3) #100
-** malloc count: 1
+** malloc count: 100


This is unfortunate.

Very much so. The two possible fixes I can think of would be to get multiple specialisation working for the recursive call, or to turn the constant back into code to create a fresh structure when that would allow the update to be destructive. Both of these seem pretty difficult to wrangle, so I put in the PR without fixing this.

jimbxb · 2024-12-18T20:15:11Z

src/LLVM.hs

+    return $ Just $ FloatStructMember n (sz `div` 8)
+argConstValue ArgClosure{} = return Nothing -- const closures already handled
+argConstValue (ArgGlobal info _) = do
+    -- XXX is ArgGlobal a constant?  Does it give the address, or value, of the


I think this has come up before. I can't recall exactly what I thought last time, but I think that the address makes sense -- in order to get the value you must load.

jimbxb · 2024-12-18T20:18:51Z

src/AST.hs

+-- | The LLVM constant name for the specified StructID
+structConstName :: StructID -> Ident
+structConstName (StructID md num _) =
+    specialName2 (showModSpec md) $ "constant" ++ specialChar : show num


We need a specialName{N} variant, for N > 2. Seems annoying...

Maybe

specialNames :: [String] -> String specialNames = intercalate [specialChar]

is more ergonomic.

jimbxb · 2024-12-18T20:19:43Z

src/LLVM.hs

-tryLookupConstant spec =
-    gets $ Map.lookup spec . constNames
+writeConstDeclaration :: StructID -> LLVM ()
+-- writeConstDeclaration spec@(WybeStringSpec str) n = do


jimbxb · 2024-12-18T20:20:49Z

src/LLVM.hs

@@ -785,7 +817,7 @@ writeWybeCall wybeProc args pos = do
 -- | Generate a Wybe proc call instruction, or defer it if necessary.
 writeHOCall :: PrimArg -> [PrimArg] -> OptPos -> LLVM ()
 writeHOCall (ArgClosure pspec closed _) args pos = do
-    -- NB:  this case doesn't seem to ever occur
+    -- NB:  this case doesn't seem to ever occur -- probably handled earlier


That's right. The body builder should always lower a call to a known higher order term to a first order call. I think I added this special case a while back, probably before I tackled it in the body builder! Should be safe to remove.

jimbxb · 2024-12-18T20:21:50Z

src/LLVM.hs

-typeRep ty =
-    trustFromJust ("lookupTypeRepresentation of unknown type " ++ show ty)
-      <$> lift (lookupTypeRepresentation ty)
+typeRep = lift . typeRepresentation


Do we really need this shorthand now?

jimbxb · 2024-12-18T20:22:55Z

src/LLVM.hs

                                     -- ^ Static constants appearing in module
-        constNames :: Map StaticConstSpec Ident,
-                                    -- ^ local name given to static constants
+        -- constNames :: Map StaticConstSpec Ident,


jimbxb · 2024-12-19T05:00:36Z

src/BodyBuilder.hs

+                    Map.insert var (newBlock size) $ constStructs st}
+
+
+-- | Does the specified variable hold a constant structure?


Doc is wrong here

jimbxb · 2024-12-20T12:06:02Z

src/AST.hs

+stringExpr str = do
+    cStringArg <- Unplaced <$> cStringExpr str
+    return $ Typed
+            (Fncall ["wybe","string"] "unsafe_cstring_to_string" False


Can we just make the buffer ctor pub? Or do you like the name having unsafe?

jimbxb · 2024-12-20T12:08:37Z

src/AST.hs

+-- | Return the representation of a StructMember
+constValueRepresentation :: ConstValue -> TypeRepresentation
+constValueRepresentation (IntStructMember _ size) = Bits $ size * 8
+constValueRepresentation (FloatStructMember _ size) = Floating $ size * 8


I recall having constants for these 8s. Can we use them here too, for clarity?

jimbxb · 2024-12-20T12:11:56Z

src/AST.hs

@@ -3761,6 +4020,18 @@ envPrimParam = PrimParam envParamName AnyType FlowIn Ordinary (ParamInfo False e
 makeGlobalResourceName :: ResourceSpec -> String
 makeGlobalResourceName spec = specialName2 "resource" $ show spec

+-- These are defined in Snippets.hs, but we can't import that as it imports AST


Non-recursive modules is painful 😖

jimbxb · 2024-12-20T12:22:04Z

src/BodyBuilder.hs

+    case members of
+      [(n,PointerStructMember id),(0,IntStructMember len _)] ->
+        lift (lookupConstInfo id) >>= \case
+          Just (CStringInfo str) | len == fromIntegral (length str) -> do


Is there ever a case where this isn't true?

I'm overall not sure why we need this...

jimbxb · 2024-12-20T12:50:28Z

I'm reviewing this via my phone at the airport, so can't access my laptop. Unfortunately, it seems that after a certain number of files, GitHub refuses to load diffs. I noticed, by chance in one of the commits a change that is odd. This should be the reference to it:

https://github.com/pschachte/wybe/blob/const_structs/test-cases%2Ffinal-dump%2Fearly_error.exp#L74

Any chance we can avoid these changes? I assume the issue is before types, so perhaps in Flatten/Normalise, where string constants are replaced with tmp variables (that hold the constant value). I think this is also causing one of the error messages changing too, that I commented on before.

jimbxb · 2024-12-20T12:42:15Z

test-cases/final-dump/bug228-type.exp

@@ -1,4 +1,4 @@
 Error detected during type checking of module(s) bug228-type
 final-dump/bug228-type.wybe:5:2: Type error in call to proc =, argument 1
 final-dump/bug228-type.wybe:5:2: Type error in call to proc =, argument 2
-final-dump/bug228-type.wybe:5:2: Type of "1.0" incompatible with ?bar
+final-dump/bug228-type.wybe:5:2: Type of tmp#0:wybe.string incompatible with ?bar


Thus is unfortunate

jimbxb · 2024-12-27T04:46:38Z

src/Flatten.hs

    return $ typeAndPlace expr ty castFrom pos
+flattenExp expr@(StringValue str WybeString) ty castFrom pos = do
+    exp' <- lift $ stringExpr str


I suppose this is what is causing the errors around string constants, as extra "unreachable" statements are introduced.

I don't know how we can avoid this, unfortunately. Perhaps we can delay this until Clause? That has implications for the const structures that are referenced in the module, I believe.

pschachte added 22 commits October 22, 2024 18:17

Eliminate ArgChar PrimArg constructor

5538926

Replacing uses with ArgInt. This conversion happened anyway, later in the compiler, so not much is gained by having that PrimArg constructor. Huge number of changes to .exp files, but most are trivial changes to putchar calls.

Move optimisation of constant casts to more appropriate place

9feb282

Partially working. Infers const structs.

759866d

Still generates bogus code in some cases.

Add XXX comment to BodyBuilder.hs

e9132fb

Treat constant structs as aliased

7ca965a

Also recognise that constant structs are always not equal to 0

Convert constant closures into constant structures

0b9d5fe

Extract GlobalFlows from const structures

c880711

Optimise access of constant struct; allow conversion CPointer -> Signed

f7c79ea

cleanup

ef17b21

allow mutate of constant struct to produce another constant struct; tidy

8b5df6b

Mark some expected output changes correct

801dd6c

Improve doc of BodyBuilder.hs

e123125

Rebuild src/README.md

ab59b0f

Some changed expected outputs

098e788

Accept more test case outputs

0ca80ad

Fix problem with LLVM aligning undef ints for fill

15209e0

LLVM aligns structure fields, so we can't just use a single undef int the right size to fill in holes in a structure. Instead, we allocate the needed number of 1-byte undefs, plus the needed number of word-size undefs.

Fix oversight scanning GenericStructMembers

e977fd0

Improve dumping and logging of string constants

76e37e3

Retain cast information when flattening

5da5be3

Fix regression in handling explicit typing

2670859

Fix bugs in flattening code with explicit types

6cc278f

Optimising constant closures already handled by bodybuilder

65c77d9

pschachte requested a review from jimbxb December 12, 2024 05:31

jimbxb reviewed Dec 12, 2024

View reviewed changes

pschachte added 2 commits December 15, 2024 17:01

Add XXX comment about specialisation of specialisations.

76d92f8

small doc change

27494a8

jimbxb requested changes Dec 18, 2024

View reviewed changes

jimbxb reviewed Dec 20, 2024

View reviewed changes

jimbxb reviewed Dec 27, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Const structs #476

Const structs #476

pschachte commented Dec 12, 2024

jimbxb left a comment

jimbxb Dec 12, 2024

jimbxb Dec 18, 2024

jimbxb Dec 12, 2024

jimbxb Dec 12, 2024

pschachte Dec 12, 2024

jimbxb Dec 18, 2024

jimbxb Dec 18, 2024

jimbxb Dec 18, 2024

jimbxb Dec 18, 2024 •

edited

Loading

jimbxb Dec 18, 2024

jimbxb Dec 18, 2024

jimbxb Dec 19, 2024

jimbxb Dec 20, 2024

jimbxb Dec 20, 2024

jimbxb Dec 20, 2024

jimbxb Dec 20, 2024

jimbxb commented Dec 20, 2024

jimbxb Dec 20, 2024

jimbxb Dec 27, 2024

		Map.insert var (newBlock size) $ constStructs st}


		-- \| Does the specified variable hold a constant structure?

Const structs #476

Are you sure you want to change the base?

Const structs #476

Conversation

pschachte commented Dec 12, 2024

jimbxb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimbxb Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimbxb commented Dec 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimbxb Dec 18, 2024 •

edited

Loading