Skip to content

[NVPTX] Implement isTruncateFree and isZExtFree for i32/i64 Optimizations #114683

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.

Sign up for GitHub

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

Closed
wants to merge 4 commits into from
Closed

[NVPTX] Implement isTruncateFree and isZExtFree for i32/i64 Optimizations #114683

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f43171b
Implemented isZextFree and IsTruncateFree in NVPTX target lowering.
SergeantCooper Nov 2, 2024
401f834
fixed the implementations of istruncatefree and iszextfree according …
SergeantCooper Nov 2, 2024
519453e
Merge branch 'llvm:main' into dev
SergeantCooper Nov 6, 2024
6550044
Implemented the changes based on the feedback accordingly.
SergeantCooper Nov 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3340,6 +3340,23 @@ bool NVPTXTargetLowering::splitValueIntoRegisterParts(
return false;
}

bool llvm::NVPTXTargetLowering::isTruncateFree(EVT FromVT, EVT ToVT) const {
if (FromVT.isVector() || ToVT.isVector() || !FromVT.isInteger() ||
!ToVT.isInteger()) {
Copy link
Member

@Artem-B Artem-B Nov 4, 2024

Choose a reason for hiding this comment

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

Style nit. LLVM prefers no braces around single-statement bodies.
https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

Also, my personal nit: I prefer minimizing the number of negations. .. || !(FromVT.isInteger() && ToVT.isInteger()) is, IMO, better at conveying the intent ( as in "we want both to be integers") instead of eliminating unwanted parts piecemeal.

return false;
}

return FromVT.getSizeInBits() == 64 && ToVT.getSizeInBits() == 32;
}

bool llvm::NVPTXTargetLowering::isZExtFree(EVT FromVT, EVT ToVT) const {
return false;
}

bool llvm::NVPTXTargetLowering::isZExtFree(Type *SrcTy, Type *DstTy) const {
return false;
}
Copy link
Contributor

@justinfargnoli justinfargnoli Nov 4, 2024

Choose a reason for hiding this comment

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

I'm following up offline on what our approach for these should be. I'll update this thread later.


Copy link
Contributor

@justinfargnoli justinfargnoli Nov 4, 2024

Choose a reason for hiding this comment

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

First, let's move the implementation of these functions to NVPTXISelLowering.h. They are small enough that it should be okay, and it's the existing convention.

// This creates target external symbol for a function parameter.
// Name of the symbol is composed from its index and the function name.
// Negative index corresponds to special parameter (unsized array) used for
Expand Down
4 changes: 4 additions & 0 deletions llvm/lib/Target/NVPTX/NVPTXISelLowering.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -616,6 +616,10 @@ class NVPTXTargetLowering : public TargetLowering {
return true;
}

bool isTruncateFree(EVT FromVT, EVT ToVT) const override;
bool isZExtFree(EVT FromVT, EVT ToVT) const override;
bool isZExtFree(Type *SrcTy, Type *DstTy) const override;
Copy link
Contributor

@justinfargnoli justinfargnoli Nov 4, 2024

Choose a reason for hiding this comment

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

Let's move this up to where the existing implementation of isTruncateFree() resides.


private:
const NVPTXSubtarget &STI; // cache the subtarget here
SDValue getParamSymbol(SelectionDAG &DAG, int idx, EVT) const;
Expand Down
17 changes: 17 additions & 0 deletions llvm/test/CodeGen/NVPTX/truncate_zext.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
; RUN: llc -march=nvptx64 < %s | FileCheck %s
Copy link
Member

@Artem-B Artem-B Nov 4, 2024

Choose a reason for hiding this comment

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

You may want to use llvm/utils/update_llc_test_checks.py to generate the checks.


Copy link
Contributor

@justinfargnoli justinfargnoli Nov 4, 2024

Choose a reason for hiding this comment

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

This test looks like it's checking that the correct PTX is generated for trunc and zext. This is of course nice to have, but not what we need for this PR.

The purpose of the test associated with this PR should be to ensure that your implementation of isTruncateFree() has an impact. Thus, the test should fail without your change and pass with it.

I like to find a place where isTruncateFree(EVT, EVT) is used in a simple peephole and check against that. For example, you could piggyback off of this peephole (Added and tested in 3332b70).

Copy link
Member

@Artem-B Artem-B Nov 4, 2024

Choose a reason for hiding this comment

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

+1 to that. The test should have some code which has to choose between free ops vs an alternative which would be used otherwise (e.g. i32 trunc(add(i64,i64)) vs i32 add(trunc(i64), trunc(i64)))

; Test for truncation from i64 to i32
define i32 @test_trunc_i64_to_i32(i64 %val) {
; CHECK-LABEL: test_trunc_i64_to_i32
; CHECK: trunc
%trunc = trunc i64 %val to i32
ret i32 %trunc
}

; Test for zero-extension from i32 to i64
define i64 @test_zext_i32_to_i64(i32 %val) {
; CHECK-LABEL: test_zext_i32_to_i64
; CHECK: zext
%zext = zext i32 %val to i64
ret i64 %zext
}
Copy link
Contributor

@justinfargnoli justinfargnoli Nov 4, 2024

Choose a reason for hiding this comment

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

Add a new line at the end of the file.

Loading