This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] Add codegen test for wasm_simd128.h
ClosedPublic

Authored by tlively on May 3 2021, 5:42 PM.

Details

Summary

We previously did not have tests demonstrating that the intrinsics in
wasm_simd128.h lower to reasonable LLVM IR. This commit adds such a test.

Diff Detail

Event Timeline

tlively created this revision.May 3 2021, 5:42 PM
tlively requested review of this revision.May 3 2021, 5:42 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 3 2021, 5:42 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
aheejin accepted this revision.May 4 2021, 12:56 AM
This revision is now accepted and ready to land.May 4 2021, 12:56 AM

how's the runtime of this test compared to others in clang? Is it worth splitting it up to improve test execution parallelism? (I'm guessing /probably not/ (guess while the file is long, it's relatively simple for the compiler compared to some other non-trivial template recursion tests, etc), but wanted to check)

At just under 4 seconds on my machine, it's certainly not one of the quickest clang lit tests, but neither is it one of the slowest. It looks like there are many that take tens of seconds.

This revision was automatically updated to reflect the committed changes.

At just under 4 seconds on my machine, it's certainly not one of the quickest clang lit tests, but neither is it one of the slowest. It looks like there are many that take tens of seconds.

Yep, sounds alright - thanks for the data!

hans added a subscriber: hans.May 5 2021, 2:43 AM

The test was failing in no-asserts builds, see output at https://ghostbin.com/paste/UMtJl

I put "REQUIRES: asserts" on it in 4f4aa7b78df5544b0a1c07ee98475939c1175990 for now.

Thanks, @hans! That's very surprising; I'll take a deeper look at what was going wrong.

Thanks, @hans! That's very surprising; I'll take a deeper look at what was going wrong.

Labels (& instructions/values) are only named in ASSERTS enabled builds (or you can opt into them more narrowly with some other build config)

eg:
With asserts:

; Function Attrs: nounwind uwtable
define dso_local void @func(i32 %b, i32 %i, i32 %j) local_unnamed_addr #0 {
entry:
  %tobool.not = icmp eq i32 %b, 0
  br i1 %tobool.not, label %if.end, label %if.then

if.then:                                          ; preds = %entry
  tail call void (...) @f1() #2
  br label %if.end

if.end:                                           ; preds = %if.then, %entry
  ret void
}

Without asserts:

; Function Attrs: nounwind uwtable
define dso_local void @func(i32 %0, i32 %1, i32 %2) local_unnamed_addr #0 {
  %4 = icmp eq i32 %0, 0
  br i1 %4, label %6, label %5

5:                                                ; preds = %3
  tail call void (...) @f1() #2
  br label %6

6:                                                ; preds = %5, %3
  ret void
}

@dblaikie, what's the best practice for making tests robust to this difference?

@dblaikie, what's the best practice for making tests robust to this difference?

If you need a label or register (eg: if you want to check a branch branches to some particular spot) - the first mention of the label or register should use a pattern match (which I think this test already do generally for instructions/values - but not for labels)

I'm not sure there's a canonical way to skip over the unused "entry" label in an asserts build while not tripping up the non-asserts build (perhaps you could look around at similar tests) - I guess probably not & people accept a bit of looseness in the tests by not using CHECK-NEXT for that first instruction/line in the test. You could add a CHECK-NOT (or --implicit-check-not) maybe for " = " which would catch some instructions (not all of them - those that don't return a value, for instance) if they snuck in between the start of the function and the first instruction the test was checking for.