Page MenuHomePhabricator

[Verifier] Additional check for get.active.lane.mask
ClosedPublic

Authored by SjoerdMeijer on Aug 20 2020, 9:15 AM.

Details

Summary

Adapt the verifier checks for get.active.lane.mask to the new semantics as proposed in D86147.
I.e., we expect second argument %n to be greater than 0, so check that.

Diff Detail

Event Timeline

SjoerdMeijer created this revision.Aug 20 2020, 9:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 20 2020, 9:15 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
SjoerdMeijer requested review of this revision.Aug 20 2020, 9:15 AM
samparker added inline comments.Aug 24 2020, 3:51 AM
llvm/test/Verifier/get-active-lane-mask.ll
24

Tests for (i32 1) and (i32 -1) as well?

Yep, thanks, now with extra tests for (i32 1) and (i32 -1).

samparker added inline comments.Aug 25 2020, 12:05 AM
llvm/test/Verifier/get-active-lane-mask.ll
36

I'm not familiar with verifying tests.. but why isn't this checking for the same result as the zero input case?

SjoerdMeijer added inline comments.Aug 25 2020, 12:36 AM
llvm/test/Verifier/get-active-lane-mask.ll
36

because we're checking %TC > 0 here, so 0 should throw a verifier message, and all the other values should be accepted.

samparker added inline comments.Aug 25 2020, 12:40 AM
llvm/test/Verifier/get-active-lane-mask.ll
36

So.... shouldn't -1 also throw the same error?

SjoerdMeijer added inline comments.Aug 25 2020, 12:48 AM
llvm/test/Verifier/get-active-lane-mask.ll
36

Ah, sorry, this comment is just about the -1 case, I didn't read properly and thought it was about the 1 and -1 cases.

The -1 is interpreted as an unsigned value (so is a big positive number). I think that's what we want because the semantics of the intrinsics is an icmp ult.

samparker added inline comments.Aug 25 2020, 1:07 AM
llvm/test/Verifier/get-active-lane-mask.ll
36

Okay, seems a little counter-intuitive after reading the assertion code, it looks like we could just check against a zero value instead of doing zext(TC) > 0.

Cheers, that's indeed a bit shorter.

samparker accepted this revision.Aug 25 2020, 2:04 AM

Thanks, LGTM

This revision is now accepted and ready to land.Aug 25 2020, 2:04 AM

This check isn't appropriate. Given that the second argument to @llvm.get.active.lane.mask.v4i1.i32 is a variable, in general, it isn't appropriate to print an error in cases where we can prove the value is 0. You can add a Lint check if you want.

Also, if @llvm.get.active.lane.mask.v4i1.i32(i32 0, i32 0) is supposed to produce poison, that isn't documented at all.

This check isn't appropriate. Given that the second argument to @llvm.get.active.lane.mask.v4i1.i32 is a variable, in general, it isn't appropriate to print an error in cases where we can prove the value is 0. You can add a Lint check if you want.

Thanks for pointing this out, I would not have guessed that we shouldn't verify the cases that we can verify.... Just for my understanding, before I revert this, can you explain this? I do see of course that variables and constants get a different treatment here. Is it this discrepancy that makes this not appropriate?

Also, if @llvm.get.active.lane.mask.v4i1.i32(i32 0, i32 0) is supposed to produce poison, that isn't documented at all.

We have added that %n > 0 to the definition, but it sounds like I need to add " and it produces poison values otherwise". I will create a diff for that.

The problem with checking for 0 is that it makes a bunch of transforms much harder. For example, if you have "add i32 0, 0", we want to fold that to "i32 0". With this verifier check, every pass that does this sort of transform would need to check if the value is used by a llvm.get.active.lane.mask() call.

The Lint pass exists to find code that is technically valid, but not sane.

Ah yes, thanks Eli!
This is reverted in rG1d8af682ef1d, and I will move this to Lint.