== with Option<primitive> has two branches

[huonw]

pub fn foo(a: Option<u8>) -> bool {
    a == Some(2)
}

optimises to (with --opt-level=3 --crate-type=lib)

define i8 @_ZN3foo20he42f37a9b93a7ee3daa4v0.0E(%"enum.std::option::Option<u8>[#1]") unnamed_addr #0 {
entry-block:
  %.fca.0.extract8 = extractvalue %"enum.std::option::Option<u8>[#1]" %0, 0
  %cond.i = icmp eq i8 %.fca.0.extract8, 0
  br i1 %cond.i, label %"_ZN6option30Option$LT$T$GT$...std..cmp..Eq2eq20h54698946061538303014v0.0E.exit", label %match_case9.i

match_case9.i:                                    ; preds = %entry-block
  %.fca.2.0.extract10 = extractvalue %"enum.std::option::Option<u8>[#1]" %0, 2, 0
  %1 = icmp eq i8 %.fca.2.0.extract10, 2
  %2 = zext i1 %1 to i8
  br label %"_ZN6option30Option$LT$T$GT$...std..cmp..Eq2eq20h54698946061538303014v0.0E.exit"

"_ZN6option30Option$LT$T$GT$...std..cmp..Eq2eq20h54698946061538303014v0.0E.exit": ; preds = %entry-block, %match_case9.i
  %__make_return_pointer.0.i = phi i8 [ %2, %match_case9.i ], [ 0, %entry-block ]
  ret i8 %__make_return_pointer.0.i
}

In a perfect world, this would optimise to something like transmute::<_, u16>(a) == 0x0102.

[zkamsler]

That would be nice to have, although I can think of a few issues with such an approach:

1. The Option may have lower alignment requirements than the integer of the same size (i.e. {u8,u8} is usually 1-byte aligned and u16 is usually 2-byte aligned). That may cause issues on platforms that do not handle unaligned loads, but I don't know enough to know if transmute handles that.
2. There may be padding between the discriminant and the value, which I assume could be uninitialized.

[huonw]

1. The Option may have lower alignment requirements than the integer of the same size (i.e. {u8,u8} is usually 1-byte aligned and u16 is usually 2-byte aligned). That may cause issues on platforms that do not handle unaligned loads, but I don't know enough to know if transmute handles that.

Seems like a possible problem, yeah.

1. There may be padding between the discriminant and the value, which I assume could be uninitialized.

Oh, I thought I mentioned this (it's why I chose to use Option<u8> in the example). In any case, for x: Option<u16>, I'd guess x == Some(2) becoming

transmute::<_, u32>(x) & 0xff00_ffff == 0x0100_0002

may be faster than two branches. (However, this is becoming complicated and hard to implement/get right.)

@huonw This is what I'm getting on master today:

define zeroext i1 @_ZN3foo20h8f1b2f0c18884ca6daaE(%"enum.core::option::Option<[u8]>[#3]") unnamed_addr #0 {
match_case5.i:
  %.fca.0.extract7 = extractvalue %"enum.core::option::Option<[u8]>[#3]" %0, 0
  %.fca.2.0.extract9 = extractvalue %"enum.core::option::Option<[u8]>[#3]" %0, 2, 0
  %cond.i = icmp eq i8 %.fca.0.extract7, 1
  %1 = icmp eq i8 %.fca.2.0.extract9, 2
  %__make_return_pointer.0.i = and i1 %cond.i, %1
  ret i1 %__make_return_pointer.0.i
}

Can we close this?

[thestinger]

It's still doing more work than it should be. It's just optimizing the two checks into an AND now.

[dotdash]

The thing here is that passing the option as a struct means that LLVM passes the fields individually. So the Option's discriminant and its value are in two different registers (on x86_64). I guess the concrete way things are passed at the asm level is unknown at the IR level. The argument could be on the stack (then the alignment problems would matter), in a single register or split into multiple registers. So it's unable to optimize this any further at this level.

It can do better if it gets a pointer and you tell it that the alignment is suitable:

; Function Attrs: nounwind readnone uwtable
define zeroext i1 @_ZN3foo20h8ac12d4c0db8e3ccdaaE({ i8, i8 }*) unnamed_addr #0 {
match_case5.i:
  %p1 = getelementptr inbounds { i8, i8 }* %0, i64 0, i32 0
  %p2 = getelementptr inbounds { i8, i8 }* %0, i64 0, i32 1
  %v1 = load i8* %p1, align 2 ; Just for demonstration purposes, it's not actually aligned like that
  %v2 = load i8* %p2
  %cond.i = icmp eq i8 %v1, 1
  %1 = icmp eq i8 %v2, 2
  %sret_slot.0.i = and i1 %cond.i, %1
  ret i1 %sret_slot.0.i
}

becomes

; Function Attrs: nounwind readnone uwtable
define zeroext i1 @_ZN3foo20h8ac12d4c0db8e3ccdaaE({ i8, i8 }* nocapture readonly) unnamed_addr #0 {
match_case5.i:
  %1 = bitcast { i8, i8 }* %0 to i16*
  %v1 = load i16* %1, align 2
  %2 = icmp eq i16 %v1, 513
  ret i1 %2
}

The later passes could maybe generate slightly better asm by first combining the values into a single register and then compare against that. Not sure if that much better than the current code though.

Improving the handling of aggregate values as arguments in LLVM, so that the struct gets passed in a single register might work, but I'm not really sure about that.

What does work (of course ;-)) is adding a coercion to a primitive integer type.

; Function Attrs: noinline nounwind readnone uwtable
define internal fastcc zeroext i1 @_ZN3foo20hc5c0400b49933fc9daaE(i16 %in) unnamed_addr #0 {
match_case5.i:
  %coerce = alloca i16
  store i16 %in, i16* %coerce
  %coerce.p = bitcast i16* %coerce to %"enum.core::option::Option<[u8]>[#3]"*
  %0 = load %"enum.core::option::Option<[u8]>[#3]"* %coerce.p
  %.fca.0.extract4 = extractvalue %"enum.core::option::Option<[u8]>[#3]" %0, 0
  %.fca.2.0.extract6 = extractvalue %"enum.core::option::Option<[u8]>[#3]" %0, 2, 0
  %cond.i = icmp eq i8 %.fca.0.extract4, 1
  %1 = icmp eq i8 %.fca.2.0.extract6, 2
  %sret_slot.0.i = and i1 %cond.i, %1
  ret i1 %sret_slot.0.i
}

becomes

; Function Attrs: noinline nounwind readnone uwtable
define internal fastcc zeroext i1 @_ZN3foo20hc5c0400b49933fc9daaE(i16 %in) unnamed_addr #0 {
match_case5.i:
  %0 = icmp eq i16 %in, 513
  ret i1 %0
}

See also #16506

[dotdash]

FWIW, the coercion approach also handles padding in a struct. Option<u16> with that approach gives:

define internal fastcc zeroext i1 @_ZN3foo20hc5c0400b49933fc9daaE(i32 %in) unnamed_addr #0 {
match_case5.i:
  %0 = and i32 %in, -65281
  %1 = icmp eq i32 %0, 131073
  ret i1 %1
}