dialects: (vector) add pure trait to vector.broadcast and vector.fma

[knickish]

Add Pure trait to vector.broadcast and vector.fma, fixes #3019

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.91%. Comparing base (30dcaa1) to head (3bc7b99).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3094   +/-   ##
=======================================
  Coverage   89.91%   89.91%           
=======================================
  Files         425      425           
  Lines       53391    53394    +3     
  Branches     8268     8268           
=======================================
+ Hits        48006    48009    +3     
  Misses       4047     4047           
  Partials     1338     1338           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[superlopuh]

Thank you for this! Could you please add a minimal filecheck test that tests that these get dce'd away? Should be a minimal test with "test.op"s that produce the required inputs for the ops, and no uses, and that checks that the operations are removed after calling the dce pass

[webmiche]

Honestly, I am surprised that the broadcast op would be pure. If MLIR models it this way, it's fine to have it here aswell, I guess.

[superlopuh]

https://mlir.llvm.org/docs/Dialects/Vector/#vectorbroadcast-vectorbroadcastop
You're right

[knickish]

Is that indicated by the Effects: MemoryEffects::Effect{} line? Looks like nearly all of them have that

[superlopuh]

Yep! We now have a similar trait in xDSL, if you could add the NoMemoryEffect trait instead of Pure that would be ideal. And then also a filecheck test that check that they still get removed by dce

[PapyChacal]

@webmiche @superlopuh As I discovered similarly on #3103 (resolved discussion); MLIR tend to not report some AlwaysSpeculatable operations properly in their documentation.

See:

• https://github.com/llvm/llvm-project/blob/98e674c9f16d677d95c67bc130e267fae331e43c/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td#L348-L349
• https://github.com/llvm/llvm-project/blob/98e674c9f16d677d95c67bc130e267fae331e43c/mlir/include/mlir/Dialect/Vector/IR/VectorOps.td#L597-L598

Those ops are indeed Pure and @knickish was right 🙂

[PapyChacal]

(Plus, I personally don't think we should check traits' behaviour on dialect operations everytime, those are tested on their own through test operations, just my 2 cents on that front)

[superlopuh]

We don't need to go into a full debate here but to me they're two separate things, the trait is just one way to achieve the desired behaviour, and are an implementation detail, and I think it's worth having a test for each dialect to make sure that the operations have the expected behaviour, in this case that the operations can be eliminated if their results are not used.

[knickish]

Ok, so leave as Pure and add the filecheck tests, correct?

[superlopuh]

Yes, please :)

[knickish]

I gave the test a shot, not sure if that's exactly what it's supposed to be though

[superlopuh]

ok but now the func is gone so one more change :)

[superlopuh]

Thank you!