Skip to content

LoopIdiomRecognize: strip bad TODO (NFC) #92890

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

Merged
merged 1 commit into from
May 28, 2024
Merged

Conversation

artagnon
Copy link
Contributor

There are several reasons why handling powi in LoopIdiomRecognize is a bad idea:

  • powi corresponds to a GCC builtin that is only defined for C int (which is i32 for most targets).
  • powi isn't always lowered by targets correctly for non-i32 parameters. Several targets fail to compile llvm.powi.f32.i16, for example.
  • Unlike memcpy and memset, which tend to be important enough internal intrinsics that you have to handle them correctly even in freestanding modes, powi isn't.

Strip this bad TODO to avoid misleading contributors.

There are several reasons why handling powi in LoopIdiomRecognize is a
bad idea:

- powi corresponds to a GCC builtin that is only defined for C int
  (which is i32 for most targets).
- powi isn't always lowered by targets correctly for non-i32 parameters.
  Several targets fail to compile llvm.powi.f32.i16, for example.
- Unlike memcpy and memset, which tend to be important enough internal
  intrinsics that you have to handle them correctly even in freestanding
  modes, powi isn't.

Strip this bad TODO to avoid misleading contributors.
@llvmbot
Copy link
Member

llvmbot commented May 21, 2024

@llvm/pr-subscribers-llvm-transforms

Author: Ramkumar Ramachandra (artagnon)

Changes

There are several reasons why handling powi in LoopIdiomRecognize is a bad idea:

  • powi corresponds to a GCC builtin that is only defined for C int (which is i32 for most targets).
  • powi isn't always lowered by targets correctly for non-i32 parameters. Several targets fail to compile llvm.powi.f32.i16, for example.
  • Unlike memcpy and memset, which tend to be important enough internal intrinsics that you have to handle them correctly even in freestanding modes, powi isn't.

Strip this bad TODO to avoid misleading contributors.


Full diff: https://github.com/llvm/llvm-project/pull/92890.diff

1 Files Affected:

  • (modified) llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp (+2-4)
diff --git a/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp b/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
index c7e25c9f3d2c9..3fe5478408d45 100644
--- a/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
@@ -22,8 +22,6 @@
 //
 // Future loop memory idioms to recognize:
 //   memcmp, strlen, etc.
-// Future floating point idioms to recognize in -ffast-math mode:
-//   fpowi
 //
 // This could recognize common matrix multiplies and dot product idioms and
 // replace them with calls to BLAS (if linked in??).
@@ -1107,7 +1105,7 @@ bool LoopIdiomRecognize::processLoopStridedStore(
     GV->setAlignment(Align(16));
     Value *PatternPtr = GV;
     NewCall = Builder.CreateCall(MSP, {BasePtr, PatternPtr, NumBytes});
-    
+
     // Set the TBAA info if present.
     if (AATags.TBAA)
       NewCall->setMetadata(LLVMContext::MD_tbaa, AATags.TBAA);
@@ -1117,7 +1115,7 @@ bool LoopIdiomRecognize::processLoopStridedStore(
 
     if (AATags.NoAlias)
       NewCall->setMetadata(LLVMContext::MD_noalias, AATags.NoAlias);
-  } 
+  }
 
   NewCall->setDebugLoc(TheStore->getDebugLoc());
 

@artagnon
Copy link
Contributor Author

For context, please see #72650.

@artagnon artagnon merged commit 7bea41e into llvm:main May 28, 2024
6 checks passed
@artagnon artagnon deleted the lir-powi-todo branch May 28, 2024 16:43
vg0204 pushed a commit to vg0204/llvm-project that referenced this pull request May 29, 2024
There are several reasons why handling powi in LoopIdiomRecognize is a
bad idea:

- powi corresponds to a GCC builtin that is only defined for C int
(which is i32 for most targets).
- powi isn't always lowered by targets correctly for non-i32 parameters.
Several targets fail to compile llvm.powi.f32.i16, for example.
- Unlike memcpy and memset, which tend to be important enough internal
intrinsics that you have to handle them correctly even in freestanding
modes, powi isn't.

Strip this bad TODO to avoid misleading contributors.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants