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

[FXML-4501] Lower arith.andi, arith.ori, arith.shli, arith.shrsi, arith.shrui, arith.xori to emitc #165

Merged
merged 9 commits into from
May 29, 2024

Conversation

cferry-AMD
Copy link
Collaborator

No description provided.

@cferry-AMD cferry-AMD marked this pull request as draft April 16, 2024 14:29
@cferry-AMD cferry-AMD changed the title [FXML-4312] Lower arith.andi, arith.ori, arith.shli, arith.shrsi, arith.xori to emitc [FXML-4501] Lower arith.andi, arith.ori, arith.shli, arith.shrsi, arith.xori to emitc Apr 18, 2024
@cferry-AMD cferry-AMD force-pushed the corentin.arith_binops branch from 6323607 to 51ea141 Compare May 17, 2024 12:44
@cferry-AMD cferry-AMD changed the title [FXML-4501] Lower arith.andi, arith.ori, arith.shli, arith.shrsi, arith.xori to emitc [FXML-4501] Lower arith.andi, arith.ori, arith.shli, arith.shrsi, arith.shrui, arith.xori to emitc May 21, 2024
@cferry-AMD cferry-AMD marked this pull request as ready for review May 21, 2024 06:50
@cferry-AMD cferry-AMD requested a review from mgehre-amd May 21, 2024 06:50
@mgehre-amd
Copy link
Collaborator

C++ about shift operations:

The behavior is undefined if the right operand
is negative, or greater than or equal to the length in bits of the promoted left operand.

and the MLIR shift ops say

If the value of the second operand is greater or equal than the bitwidth of the first operand, then the operation returns poison.

Here, negative in the C++ world is the same as greater or equal in the emitc world as we cast the right operand to unsigned before.
Formally, undefined behavior is stronger than poison as it can time-travel.
I think we either need to check statically that no UB happens, or emit an runtime guard for it.

@mgehre-amd
Copy link
Collaborator

Alternatively, checking the nuw/nsw flags on arith.shli might guarantee that the shift amount is valid. I haven't looked to much which of them is applicable here.

@mgehre-amd
Copy link
Collaborator

Alternatively, checking the nuw/nsw flags on arith.shli might guarantee that the shift amount is valid. I haven't looked to much which of them is applicable here.

Ah, those flags don't constraint the shift amount. A better description of them is in the LLVM spec: https://llvm.org/docs/LangRef.html#shl-instruction

@cferry-AMD
Copy link
Collaborator Author

Static checking is only possible when the amount of the right shift is statically known, which practically means a constant. So, in the general case we'll need to introduce runtime checks.

However, I'm not sure of how to deal with anything that relies on poison values. There's no poison semantics in C/C++, and I have too little understanding of the differences between poison and undefined behavior to actually know what to do.

@cferry-AMD
Copy link
Collaborator Author

In the runtime check, I resorted to using sizeof for index types instead of rejecting them. I expect that the compiler does constant folding and strength reduction when the shift amount is a constant, to avoid introducing a shortcut.

Copy link
Collaborator

@josel-amd josel-amd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link

@TinaAMD TinaAMD left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@cferry-AMD cferry-AMD requested a review from mgehre-amd May 29, 2024 07:25
@cferry-AMD cferry-AMD enabled auto-merge (squash) May 29, 2024 08:40
@cferry-AMD cferry-AMD merged commit e452afa into feature/fused-ops May 29, 2024
3 checks passed
@cferry-AMD cferry-AMD deleted the corentin.arith_binops branch May 29, 2024 08:48
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