Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add test for EmitReducePrecisionIR #16775

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

apivovarov
Copy link
Contributor

@apivovarov apivovarov commented Sep 3, 2024

I noticed that the EmitReducePrecisionIR function from xla/service/elemental_ir_emitter.h is not covered by unit tests.

Given its non-trivial logic, I believe it should be thoroughly tested, particularly for corner cases.

Changes in this PR:

  • Declare EmitReducePrecisionIR function in xla/service/elemental_ir_emitter.h
  • Add EmitReducePrecisionIR_F16ToF8e5m2 test
  • Add EmitReducePrecisionIR_F16ToF8e4m3fn test

Related PR:

@reedwm
Copy link
Member

reedwm commented Sep 5, 2024

Maybe such tests should be in xla/tests/reduce_precision_test.cc. That way it will be tested on backends which don't use element_ir_emitter.

@apivovarov
Copy link
Contributor Author

apivovarov commented Sep 5, 2024

Maybe such tests should be in xla/tests/reduce_precision_test.cc. That way it will be tested on backends which don't use element_ir_emitter.

The function EmitReducePrecisionIR is declared in xla/service/elemental_ir_emitter.h and implemented in xla/service/elemental_ir_emitter.cc.

To test the functions declared in elemental_ir_emitter.h, XLA provides a corresponding file, xla/service/elemental_ir_emitter_test.cc.

EmitReducePrecisionIR utilizes LLVM and returns an llvm::Value (LLVM IR), which is then converted to a string representation and verified in the related tests.

In contrast, xla/tests/reduce_precision_test.cc does not rely on LLVM.

Given this, I believe that xla/service/elemental_ir_emitter_test.cc is the correct location for the EmitReducePrecisionIR tests.

@akuegel
Copy link
Member

akuegel commented Sep 5, 2024

Maybe such tests should be in xla/tests/reduce_precision_test.cc. That way it will be tested on backends which don't use element_ir_emitter.

The function EmitReducePrecisionIR is declared in xla/service/elemental_ir_emitter.h and implemented in xla/service/elemental_ir_emitter.cc.

To test the functions declared in elemental_ir_emitter.h, XLA provides a corresponding file, xla/service/elemental_ir_emitter_test.cc.

EmitReducePrecisionIR utilizes LLVM and returns an llvm::Value (LLVM IR), which is then converted to a string representation and verified in the related tests.

In contrast, xla/tests/reduce_precision_test.cc does not rely on LLVM.

Given this, I believe that xla/service/elemental_ir_emitter_test.cc is the correct location for the EmitReducePrecisionIR tests.

So we have different kind of tests:

  • End2End tests for all backends, those are in xla/tests
  • GPU specific End2End tests, those are in xla/service/gpu/tests
  • Filecheck tests, those are also in xla/service/gpu/tests

For the tests you are adding here, I think filecheck based tests that use the hlo-opt tool would be the best fit. Can you please rewrite the tests based on that? You can take a look at .hlo files in xla/service/gpu/tests to see how to use it.

@apivovarov
Copy link
Contributor Author

Thank you, Adrian and Reed, for your feedback. I’d like to provide some additional context on why I’m using the GTest framework for this test and the associated code.

The EmitReducePrecisionIR function consists of 138 lines of code and handles several specific cases:

  • When the destination mantissa is smaller than the source mantissa (adds 6 LLVM ops).
  • When the destination exponent is smaller than the source exponent (adds 7 LLVM ops).
  • NaN handling (adds 5 LLVM ops).
  • Prolog/epilog setup (adds 5 LLVM ops).

The function generates a sequence of LLVM IR operations that convert an input number in f16 format to an output in a reduced-precision f16-like format, with fewer bits for the exponent and mantissa. The result is returned as an llvm::Value*.

When working on f8E4M3 type support, it wasn’t immediately clear what LLVM IR or output I would get when reducing f16 to a 4-bit exponent and 3-bit mantissa. To explore this, I created a test that generates the LLVM IR and uses specific f16 constants as inputs.

Since the input is a constant, the entire LLVM IR can be evaluated and simplified to a final result. By calling llvm::Value*->print(), the result is folded into a final f16-like LLVM IR constant, represented as specific bits in hexadecimal format.

Note that the GPU compiler has its own separate EmitReducePrecision and EmitF16ToF8e5m2 functions, which use mlir::Value instead of llvm::Value.

The EmitReducePrecisionIR function’s test:

  • Is not an end-to-end test.
  • Is not an HLO test.
  • Is not a GPU test.

It’s a unit test for a specific C++ function (EmitReducePrecisionIR), which uses the LLVM API internally to emit LLVM IR for the CPU compiler.

Given this, I believe the test should use GTest along with the LLVM API to properly validate the result of EmitReducePrecisionIR.

@akuegel @reedwm

@akuegel
Copy link
Member

akuegel commented Sep 6, 2024

Thanks for the explanation. If it is for the CPU backend, then I think @ezhulenev might be a better reviewer.

@apivovarov
Copy link
Contributor Author

Hi Eugene,

Could you please review this PR? I've included details above explaining the necessity of the test and the reasoning behind using GTest with the LLVM API.

@ezhulenev

@apivovarov apivovarov requested review from ddunl and removed request for akuegel September 11, 2024 20:50
@hawkinsp hawkinsp removed their request for review September 19, 2024 19:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants