Page MenuHomePhabricator

[IRSim] Make sure that commutative intrinsics are treated as function calls without commutativity
ClosedPublic

Authored by AndrewLitteken on Feb 2 2022, 9:17 AM.

Details

Summary

Created to fix: https://github.com/llvm/llvm-project/issues/53537

Some intrinsics functions are considered commutative since they are performing operations like addition or multiplication. Some of these have extra parameters to provide extra information that are not part of the operation itself and are not commutative. This makes sure that if an instruction that is an intrinsic takes the non commutative path to handle this case.

Diff Detail

Event Timeline

AndrewLitteken created this revision.Feb 2 2022, 9:17 AM
AndrewLitteken requested review of this revision.Feb 2 2022, 9:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 2 2022, 9:17 AM
paquette accepted this revision.Feb 2 2022, 10:39 AM

I think this fix is fine for the purposes of getting the crash out of the way. But I think it's possible to fix this and also support commutative intrinsics at the same time.

In IntrinsicInst.h:

/// Return true if swapping the first two arguments to the intrinsic produces
/// the same result.
bool isCommutative() const {
   ...

We know that the first two arguments are the only ones that matter. So, maybe we can teach the similarity identifier to understand that? (The commutative case for intrinsics was added in D86798)

This revision is now accepted and ready to land.Feb 2 2022, 10:39 AM

I think we should be able to do that, right now it's an either or for the commutative path vs the non commutative path, but I think we should be give the similarity identifier a sublist of values that are commutative and a sublist that are noncommutative and run both functions.

Do you think you could add a TODO for commutative intrinsic support?

This revision was landed with ongoing or failed builds.Feb 2 2022, 11:25 AM
This revision was automatically updated to reflect the committed changes.