This is an archive of the discontinued LLVM Phabricator instance.

Add logic for math.powf -> spirv for some previously non covered cases
ClosedPublic

Authored by dan-garvey on May 9 2023, 4:40 PM.

Diff Detail

Event Timeline

dan-garvey created this revision.May 9 2023, 4:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 9 2023, 4:40 PM
dan-garvey requested review of this revision.May 9 2023, 4:40 PM
qedawkins requested changes to this revision.May 9 2023, 4:44 PM
qedawkins added a subscriber: qedawkins.

This will need a test in mlir/test/Conversion/MathToSPIRV/math-to-gl-spirv.mlir.

This revision now requires changes to proceed.May 9 2023, 4:44 PM

A few additional comments. Otherwise looks good to me after updating the test.

mlir/lib/Conversion/MathToSPIRV/MathToSPIRV.cpp
318

nit: add period to the end of comment.

319

I think it would make more sense to just convert to i32 because we don't know whether something like f16 -> int16 will be supported (@antiagainst to confirm).

320

nit: unused variable

338–349

It is probably cleaner to get the Attribute inside the if and then create the constant based on that attribute.

antiagainst requested changes to this revision.May 9 2023, 5:42 PM

Thanks for the fix! A few comments in addition to the missing test.

mlir/lib/Conversion/MathToSPIRV/MathToSPIRV.cpp
308

Nit: // Get the scalar float type.

309

Nit: Name this as scalarFloatType to be consistent with scalarIntType.

318

Nit: // Get int type of the same bitwidth and shape as the float type.

319

Nit: Type intType = scalarIntType to avoid duplication .

334

Could you add a TODO here to mention that "The following just forcefully casts y into an integer value in order to properly propagate the sign, assuming integer y cases. It doesn't cover other cases and should be fixed."

339

Can we repurpose getScalarOrVectorI32Constant to handle more bitwidths and use it here?

351

In Vulkan, "For the OpSRem and OpSMod instructions, if either operand is negative the result is undefined." https://registry.khronos.org/vulkan/specs/1.3-extensions/html/chap50.html Also, mod is an expensive operation. Here we just want to check whether it's odd or even. You can do bit operation to bitwise and 1 == 1.

dan-garvey updated this revision to Diff 520873.May 9 2023, 5:44 PM

fixed the lit test

antiagainst added inline comments.May 9 2023, 5:45 PM
mlir/lib/Conversion/MathToSPIRV/MathToSPIRV.cpp
319

yup good point. we can always convert to i32 for capability reasons. (the exact value doesn't matter that much given we only care about the odd/even part.)

dan-garvey updated this revision to Diff 520881.May 9 2023, 6:33 PM

addressed comments

dan-garvey updated this revision to Diff 520882.May 9 2023, 6:36 PM
dan-garvey marked 9 inline comments as done.

clang-format

dan-garvey updated this revision to Diff 520883.May 9 2023, 6:38 PM
dan-garvey marked 2 inline comments as done.

added periods to some comments that I missed. Sorry for spamming your inboxes = (

dan-garvey updated this revision to Diff 520887.May 9 2023, 7:30 PM

lit test fixes

dan-garvey updated this revision to Diff 520903.May 9 2023, 9:22 PM

added changed to vector test and fixed both

antiagainst accepted this revision.May 9 2023, 9:25 PM
antiagainst added inline comments.
mlir/lib/Conversion/MathToSPIRV/MathToSPIRV.cpp
339

Remaining debugging dump(). :) I'll remove these before landing.

qedawkins accepted this revision.May 9 2023, 9:25 PM
This revision is now accepted and ready to land.May 9 2023, 9:25 PM
dan-garvey updated this revision to Diff 520905.May 9 2023, 9:44 PM

removes debug dumps

Should I use Daniel Garvey <dan@nod-labs.com> as the author for landing the patch?

This revision was landed with ongoing or failed builds.May 9 2023, 9:51 PM
This revision was automatically updated to reflect the committed changes.