[PATCH] D81728: [InstCombine] Add target-specific inst combining

[Sebastian Neubauer via Phabricator via cfe-commits]

Flakebi added a comment.

Thanks for the notification @davezarzycki, an auto-bisecting bot is cool!

This failure should be fixed in b99898c1e9c5d8bade1d898e84604d3241b0087c 
.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81728/new/

https://reviews.llvm.org/D81728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81728: [InstCombine] Add target-specific inst combining

[David Zarzycki via Phabricator via cfe-commits]

davezarzycki added a comment.

I have a multi-stage, auto-git-bisecting bot that has identifying this commit 
as the source of a regression on Fedora 32 (x86-64). This commit broke my first 
stage test (release, no asserts). Might a quick fix happen or do we need to 
revert this?

  FAIL: Clang :: CodeGen/aarch64-bf16-ldst-intrinsics.c (7188 of 67650)
   TEST 'Clang :: CodeGen/aarch64-bf16-ldst-intrinsics.c' 
FAILED 
  Script:
  --
  : 'RUN: at line 1';   /tmp/_update_lc/r/bin/clang -cc1 -internal-isystem 
/tmp/_update_lc/r/lib/clang/12.0.0/include -nostdsysteminc -triple 
aarch64-arm-none-eabi -target-feature +neon -target-feature +bf16   -O2 
-emit-llvm /home/dave/ro_s/lp/clang/test/CodeGen/aarch64-bf16-ldst-intrinsics.c 
-o - | /tmp/_update_lc/r/bin/FileCheck 
/home/dave/ro_s/lp/clang/test/CodeGen/aarch64-bf16-ldst-intrinsics.c 
--check-prefixes=CHECK,CHECK64
  : 'RUN: at line 3';   /tmp/_update_lc/r/bin/clang -cc1 -internal-isystem 
/tmp/_update_lc/r/lib/clang/12.0.0/include -nostdsysteminc -triple 
armv8.6a-arm-none-eabi -target-feature +neon -target-feature +bf16 -mfloat-abi 
hard   -O2 -emit-llvm 
/home/dave/ro_s/lp/clang/test/CodeGen/aarch64-bf16-ldst-intrinsics.c -o - | 
/tmp/_update_lc/r/bin/FileCheck 
/home/dave/ro_s/lp/clang/test/CodeGen/aarch64-bf16-ldst-intrinsics.c 
--check-prefixes=CHECK,CHECK32
  --
  Exit Code: 1
  
  Command Output (stderr):
  --
  /home/dave/ro_s/lp/clang/test/CodeGen/aarch64-bf16-ldst-intrinsics.c:14:13: 
error: CHECK32: expected string not found in input
  // CHECK32: %1 = load <4 x bfloat>, <4 x bfloat>* %0, align 2
  ^
  :7:52: note: scanning from here
  define arm_aapcs_vfpcc <4 x bfloat> @test_vld1_bf16(bfloat* readonly %ptr) 
local_unnamed_addr #0 {
 ^
  :10:5: note: possible intended match here
   %vld1 = tail call <4 x bfloat> @llvm.arm.neon.vld1.v4bf16.p0i8(i8* %0, i32 2)
  ^
  /home/dave/ro_s/lp/clang/test/CodeGen/aarch64-bf16-ldst-intrinsics.c:23:13: 
error: CHECK32: expected string not found in input
  // CHECK32: %1 = load <8 x bfloat>, <8 x bfloat>* %0, align 2
  ^
  :18:53: note: scanning from here
  define arm_aapcs_vfpcc <8 x bfloat> @test_vld1q_bf16(bfloat* readonly %ptr) 
local_unnamed_addr #2 {
  ^
  :21:5: note: possible intended match here
   %vld1 = tail call <8 x bfloat> @llvm.arm.neon.vld1.v8bf16.p0i8(i8* %0, i32 2)
  ^
  
  Input file: 
  Check file: 
/home/dave/ro_s/lp/clang/test/CodeGen/aarch64-bf16-ldst-intrinsics.c
  
  -dump-input=help explains the following input dump.
  
  Input was:
  <<
  1: ; ModuleID = 
'/home/dave/ro_s/lp/clang/test/CodeGen/aarch64-bf16-ldst-intrinsics.c'
  2: source_filename = 
"/home/dave/ro_s/lp/clang/test/CodeGen/aarch64-bf16-ldst-intrinsics.c"
  3: target datalayout = 
"e-m:e-p:32:32-Fi8-i64:64-v128:64:128-a:0:32-n32-S64"
  4: target triple = "armv8.6a-arm-none-eabi"
  5:
  6: ; Function Attrs: nounwind readonly
  7: define arm_aapcs_vfpcc <4 x bfloat> @test_vld1_bf16(bfloat* 
readonly %ptr) local_unnamed_addr #0 {
  check:14'0
X~~ error: no match found
  8: entry:
  check:14'0 ~~
  9:  %0 = bitcast bfloat* %ptr to i8*
  check:14'0 ~
 10:  %vld1 = tail call <4 x bfloat> 
@llvm.arm.neon.vld1.v4bf16.p0i8(i8* %0, i32 2)
  check:14'0 
~~
  check:14'1 ?  
possible intended match
 11:  ret <4 x bfloat> %vld1
  check:14'0 ~~~
 12: }
  check:14'0 ~
 13:
  check:14'0 ~
 14: ; Function Attrs: argmemonly nounwind readonly
  check:14'0 ~~
 15: declare <4 x bfloat> @llvm.arm.neon.vld1.v4bf16.p0i8(i8*, i32) 
#1
  check:14'0 
~
 16:
  check:14'0 ~
 17: ; Function Attrs: nounwind readonly
  check:14'0 ~~~
 18: define arm_aapcs_vfpcc <8 x bfloat> @test_vld1q_bf16(bfloat* 
readonly %ptr) local_unnamed_addr #2 {
  check:14'0 
  check:23'0 
X~~ error: no match found
 19: entry:
  check:23'0 ~~
 20:  %0 = bitcast bfloat* %ptr to i8*
  check:23'0 ~
 21:  %vld1 = tail call <8 x bfloat> 
@llvm.arm.neon.vld1.v8bf16.p0i8(i8* %0, i32 

[PATCH] D81728: [InstCombine] Add target-specific inst combining

[Nicolai Hähnle via Phabricator via cfe-commits]

nhaehnle accepted this revision.
nhaehnle added a comment.
This revision is now accepted and ready to land.

This has had a month of good review that has been addressed, I'd say it's good 
to go.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81728/new/

https://reviews.llvm.org/D81728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81728: [InstCombine] Add target-specific inst combining

[Dave Green via Phabricator via cfe-commits]

dmgreen added a comment.

Thanks


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81728/new/

https://reviews.llvm.org/D81728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81728: [InstCombine] Add target-specific inst combining

[Dave Green via Phabricator via cfe-commits]

dmgreen added inline comments.



Comment at: llvm/test/CodeGen/Thumb2/mve-intrinsics/predicates.ll:2
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: opt -instcombine %s | llc -mtriple=thumbv8.1m.main -mattr=+mve.fp 
-verify-machineinstrs -o - | FileCheck %s
+; RUN: opt -instcombine -mtriple=arm %s | llc -mtriple=thumbv8.1m.main 
-mattr=+mve.fp -verify-machineinstrs -o - | FileCheck %s
 

Please use the same triple as llc for any test with "mve" in the title.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81728/new/

https://reviews.llvm.org/D81728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81728: [InstCombine] Add target-specific inst combining

[Jay Foad via Phabricator via cfe-commits]

foad added inline comments.



Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:552-555
+  /// \returns false to not do anything target specific or true to return the
+  /// value in \p ResultI from the InstCombiner. It is possible to return null
+  /// and stop further processing of the intrinsic by writing nullptr into
+  /// \p ResultI and returning true.

Did you consider returning `std::pair`?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81728/new/

https://reviews.llvm.org/D81728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81728: [InstCombine] Add target-specific inst combining

[Sebastian Neubauer via Phabricator via cfe-commits]

Flakebi marked an inline comment as done.
Flakebi added inline comments.



Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:542
+  bool simplifyDemandedUseBitsIntrinsic(InstCombiner , IntrinsicInst ,
+APInt DemandedMask, KnownBits ,
+bool ,

nikic wrote:
> `const APInt `?
I tried to change it it to to `const APInt ` but the x86 
simplifyDemandedVectorEltsIntrinsic changes `DemandedMask`, so this function 
would have to copy it or take a non-const reference.
Looking more into it, `SimplifyAndSetOp` takes `DemandedElts` by value too.
An `APInt` consists of a `uint64_t` and an `unsigned`, so it should be 16 Byte 
in most cases. Only if the represented int is larger than 64 bit, it comes with 
an allocation. I guess copying should be fine.
If you think it should be a reference anyway, let me know and I’ll change it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81728/new/

https://reviews.llvm.org/D81728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81728: [InstCombine] Add target-specific inst combining

[Chris Lattner via Phabricator via cfe-commits]

lattner resigned from this revision.
lattner added a comment.

Please don't consider me a blocker on this patch, thank you for pushing on it!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81728/new/

https://reviews.llvm.org/D81728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81728: [InstCombine] Add target-specific inst combining

[Sebastian Neubauer via Phabricator via cfe-commits]

Flakebi marked an inline comment as done.
Flakebi added inline comments.



Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:540
+  bool instCombineIntrinsic(InstCombiner , IntrinsicInst ,
+Instruction **ResultI) const;
+  bool simplifyDemandedUseBitsIntrinsic(InstCombiner , IntrinsicInst ,

nikic wrote:
> For all three functions, the calling convention seems rather non-idiomatic 
> for InstCombine. Rather than having an `Instruction **` argument and bool 
> result, is there any reason not to have an `Instruction *` return value, with 
> nullptr indicating that the intrinsic couldn't be simplified?
Yes, the function must have the option to return a nullptr and prevent that 
`visitCallBase` is called or other code is executed after 
`instCombineIntrinsic`.
So, somehow the caller must be able to see a difference between 'do nothing, 
just continue execution' and 'return this Instruction*', where the 
`Instruction*` can also be a nullptr.
The return type could be an `optional`.

I’ll take a look at your other comments on Monday.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81728/new/

https://reviews.llvm.org/D81728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81728: [InstCombine] Add target-specific inst combining

[Nikita Popov via Phabricator via cfe-commits]

nikic added inline comments.



Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:540
+  bool instCombineIntrinsic(InstCombiner , IntrinsicInst ,
+Instruction **ResultI) const;
+  bool simplifyDemandedUseBitsIntrinsic(InstCombiner , IntrinsicInst ,

For all three functions, the calling convention seems rather non-idiomatic for 
InstCombine. Rather than having an `Instruction **` argument and bool result, 
is there any reason not to have an `Instruction *` return value, with nullptr 
indicating that the intrinsic couldn't be simplified?



Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:542
+  bool simplifyDemandedUseBitsIntrinsic(InstCombiner , IntrinsicInst ,
+APInt DemandedMask, KnownBits ,
+bool ,

`const APInt `?



Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:546
+  bool simplifyDemandedVectorEltsIntrinsic(
+  InstCombiner , IntrinsicInst , APInt DemandedElts, APInt 
,
+  APInt , APInt ,

`const APInt `?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81728/new/

https://reviews.llvm.org/D81728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81728: [InstCombine] Add target-specific inst combining

[Nicolai Hähnle via Phabricator via cfe-commits]

nhaehnle added inline comments.



Comment at: llvm/include/llvm/Transforms/InstCombine/InstCombiner.h:46
+/// combine them.
+class LLVM_LIBRARY_VISIBILITY InstCombiner {
+public:

lattner wrote:
> I would really rather not make this be a public class - this is a very thick 
> interface.  Can this be cut down to something much smaller than the 
> implementation details of InstCombine?
> 
> If you're curious for a pattern that could be followed, the MLIR AsmParser is 
> a reasonable example.  The parser is spread across a bunch of classes in the 
> lib/ directory:
> https://github.com/llvm/llvm-project/blob/master/mlir/lib/Parser/Parser.cpp
> 
> But then there is a much smaller public API exposed through a header:
> https://github.com/llvm/llvm-project/blob/master/mlir/include/mlir/IR/OpImplementation.h#L229
> 
> 
I agree with the sentiment, but note @Flakebi has split up the `InstCombiner` 
class into `InstCombiner` and `InstCombinerImpl` classes, which addresses those 
concerns already as far as I'm concerned.

Looking through the new `InstCombiner`, aside from methods that are core to the 
workings of InstCombine (modifying instructions while keeping track of the 
Worklist) and methods for accessing the analyses, what's left is:

* A bunch of static methods that should arguably just be global functions in a 
utils header somewhere.
* CreateOverflowTuple and CreateNonTerminatorUnreachable

Moving those methods feels sensible, but is likely to touch a lot of code, so I 
think it would be better to do it in a separate commit.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81728/new/

https://reviews.llvm.org/D81728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81728: [InstCombine] Add target-specific inst combining

[Chris Lattner via Phabricator via cfe-commits]

lattner requested changes to this revision.
lattner added a comment.
This revision now requires changes to proceed.

This looks like a great direction, but please make sure to minimize public 
implementation details.  We don't want the vast majority of instcombine to be 
visible outside of its library (it is hairy enough as it is :-)




Comment at: llvm/include/llvm/Analysis/TargetTransformInfo.h:29
 #include "llvm/Support/DataTypes.h"
+#include "llvm/Support/KnownBits.h"
 #include 

Can this be forward declared instead of #include'd?



Comment at: llvm/include/llvm/Transforms/InstCombine/InstCombiner.h:30
+#include "llvm/Transforms/Utils/Local.h"
+#include 
+

Please minimize #includes in general, thanks :)



Comment at: llvm/include/llvm/Transforms/InstCombine/InstCombiner.h:46
+/// combine them.
+class LLVM_LIBRARY_VISIBILITY InstCombiner {
+public:

I would really rather not make this be a public class - this is a very thick 
interface.  Can this be cut down to something much smaller than the 
implementation details of InstCombine?

If you're curious for a pattern that could be followed, the MLIR AsmParser is a 
reasonable example.  The parser is spread across a bunch of classes in the lib/ 
directory:
https://github.com/llvm/llvm-project/blob/master/mlir/lib/Parser/Parser.cpp

But then there is a much smaller public API exposed through a header:
https://github.com/llvm/llvm-project/blob/master/mlir/include/mlir/IR/OpImplementation.h#L229




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81728/new/

https://reviews.llvm.org/D81728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81728: [InstCombine] Add target-specific inst combining

[Jay Foad via Phabricator via cfe-commits]

foad added a subscriber: bogner.
foad added inline comments.



Comment at: llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:1444
+  *this, *II, DemandedElts, UndefElts, UndefElts2, UndefElts3,
+  simplifyAndSetOp, ))
+return V;

efriedma wrote:
> Is there some way we can check that an intrinsic is actually target-specific, 
> to discourage people from handling generic intrinsics in target-specific ways?
That was the intent of @bogner's rG92a8c6112c6571112e8b622bfddc7e4d1685a6fe.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81728/new/

https://reviews.llvm.org/D81728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81728: [InstCombine] Add target-specific inst combining

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

> This combines instructions, so I think it belongs into the InstCombine pass. 
> On the other hand, the f16 form of the intrinsics is not available on all 
> targets, so this combination cannot be applied unconditionally but it needs 
> to be gated depending on the target.

I don't think this is a great justification for doing anything here.  You can 
always reverse the transform in isel on targets where it isn't supported; 
adding more IR patterns increases the potential for missed optimizations.

That said, I think moving the handling for target intrinsics into the target 
makes sense as a cleanup.




Comment at: llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp:1444
+  *this, *II, DemandedElts, UndefElts, UndefElts2, UndefElts3,
+  simplifyAndSetOp, ))
+return V;

Is there some way we can check that an intrinsic is actually target-specific, 
to discourage people from handling generic intrinsics in target-specific ways?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81728/new/

https://reviews.llvm.org/D81728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81728: [InstCombine] Add target-specific inst combining

[Matt Arsenault via Phabricator via cfe-commits]

arsenm added a comment.

I think an interface usable by InstructionSimplify would be helpful too, so I 
think that would be a separate thing from TTI


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81728/new/

https://reviews.llvm.org/D81728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D81728: [InstCombine] Add target-specific inst combining

[Nicolai Hähnle via Phabricator via cfe-commits]

nhaehnle added a comment.

We've been handling target-specific intrinsics in InstCombine for a long time, 
and that's the place where they should naturally sit. This is a pretty clean 
refactoring in my opinion, I'm in favor. It's substantial enough as a change 
that it should probably receive a heads-up on llvm-dev, though.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81728/new/

https://reviews.llvm.org/D81728



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits