Update skip layer norm

[amarin16]

Update the SkipLayerNorm implementation to address issues.

[fs-eire]

Does this also fix SkipSimplifiedLayerNormalization ?

[amarin16]

Does this also fix SkipSimplifiedLayerNormalization ?

Yes, both SkipLayerNormalization and SkipSimplifiedLayerNormalization are implemented using the SkipLayerNorm class

[fajin-corp]

in terms of bug fix, the changes should have fixed the race condition for the memory buffer. @amarin16 do you see any unit test failures?

[amarin16]

in terms of bug fix, the changes should have fixed the race condition for the memory buffer. @amarin16 do you see any unit test failures?

I don't see any failures in the existing unit tests

[fs-eire]

LGTM.

[fs-eire]

The latest change is incorrect.

You need to be very careful to store a const reference from a function's parameter, unless you are quite sure about the ownership and lifecycle of the object. Specifically, in this case you stores the tensor reference as a parameter of PrePack() into a pointer of a class field. The tensor object is no longer valid in the context of OpKernel::Compute() and you are actually accessing dangling pointers.

If you want to validate the initializers, you need to do that inside PrePack() and returns a failure OrtStatus.

I can also observe this failure in the original issue, but seems it is not caught by the CI pipelines.

My suggestion:

• for this PR:
  • revert the changes introduced in bc2c820
  • validate issue [Web] 1.20.0 breaks SkipSimplifiedLayerNormalization backwards compatibility. Missing Input: model.layers.0.input_layernorm.weight #22704 is fixed
  • fix validation: only does the validation when pointer is not null (see details below)
  • merge to unblock users
• Following up:
  • create a new PR to add test cases (using initializers) and initialier validation.

EDIT: I debugged the code: the input validation accesses nullptr.

[amarin16]

Before my latest changes, I initially considered doing the validation when the gamma pointer is not null like you suggested. The problem with that is that you no longer enforce the fact that gamma is not optional. And this is not the only code that uses that helper.
An alternative is to do the validation in prepack, however that's tricky because the current logic validates prepacked and non prepacked tensors in the same if statement.

I will revert bc2c820 and come up with a different way to validate

[amarin16]

Unfortunately we need input->Shape().GetDims() in PrePack in order to do any of the validations there and we don't have access to it.

[amarin16]

I added some unit tests and manually validated that they fail if you try to access gamma when prepacked

I've addressed all of Yulong's comments, but he is currently OOF