Skip to content

Conversation

@xxxbxxx
Copy link
Contributor

@xxxbxxx xxxbxxx commented Oct 4, 2023

This is generalisation of @jacobly0 #16605 to support optional and unions.

llvm appears to assume padding bits are "properly set" when using llvm.load i4:
https://llvm.org/docs/LangRef.html#load-instruction

"When loading a value of a type like i20 with a size that is not an integral number of bytes, the result is undefined if the value was not originally written using a store of the same type."

so llvm assumes the and is redundant and doesn't generate code to mask out the bits.

  %26 = getelementptr inbounds { i4, i8 }, ptr %3, i32 0, i32 0, !dbg !3141
  %27 = load i4, ptr %26, align 1, !dbg !3141
  %28 = lshr i4 %27, 1, !dbg !3142
  %29 = and i4 %28, 7, !dbg !3142  <<< does nothing
  %30 = trunc i4 %29 to i3, !dbg !3142
  %31 = icmp eq i3 %30, -3, !dbg !3142

after this commit, this byte code is generated instead, and the hi bits are properly zeroed:

  %26 = getelementptr inbounds { i4, i8 }, ptr %3, i32 0, i32 0, !dbg !3141
  %27 = load i8, ptr %26, align 1, !dbg !3141
  %28 = trunc i8 %27 to i4, !dbg !3141
  %29 = lshr i4 %28, 1, !dbg !3142
  %30 = and i4 %29, 7, !dbg !3142
  %31 = trunc i4 %30 to i3, !dbg !3142
  %32 = icmp eq i3 %31, -3, !dbg !3142

closes #14200

also, a pair of bugs seem to be fixed now by this or previous commits -> add a test case.
closes #9674
closes #16581

Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

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

This looks ready to land to me. @jacobly0 are you on board with this?

@jacobly0
Copy link
Member

I was just trying to fix bugs, so anything that passes tests is fine with me. I'm currently only really invested in Builder, not so much llvm.zig yet.

@andrewrk
Copy link
Member

Appears to have caused #17768

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

Labels

None yet

Projects

None yet

3 participants