renamed pattern rewriter function to match the StdExpandDivs theme.

implemented the requested changes, including the pass name change from -std-to-std-lowering to -std-expand-divs.

responded to comments, including the removal of the n*m>0 test that may overflow by a logical equivalent using n and m compares only.

removed redundant includes and mlir:: qualifier, corrected the comments copied over (sorry)

@silvas Will push an update with fixes for your comment. One thing I do not know and input would be appreciated. Should this pass be added in the "mlir driver"? If so, can you recommend where the pass should be added. Thanks

Followed the suggestion to include the handling of ceil and floor (generic version supporting arbitrary pos/neg nominator/denominator) as a separate std to std pass. I also removed "auto" where easily substituted by the actual type, fixed the description of the ops as requested, and fixed a couple of other suggestions.

Added suggestion, thanks for attempting to clarify the original text, much appreciated.

Thanks, glad to see that the traits make such changes easy... Please correct Uday's nit, and its good to go.

@imaihal Thanks for the upgrade. Minor nit still remains, please fix.

Just my two cents; LE vs BE typically gives problem when a program read data with one data size, and write data with another size. So if the reader in the dense attributes respect the type size, namely read and write the data using the same format, and then generate the hex directly from the register content, then there are no issues with endianness.

Rephrasing the question.

yes please.

@bondhugula I don't believe I have commit privilege yet. Please commit for me, if you don't mind.

Completed all current requests for changes.

performed all requested changes but one. Left a clarification request for the suggestion that I am not sure to understand properly.

Update to reflect latest comments.

In D86236#2231069, @bondhugula wrote:

1. Stretch question: do you see a scenario where some operand memrefs are normalizable but not others?

Since this 'MemRefsNormalizable' trait is more of "can/may be normalized" and is a first pass check because there are other things that could prevent it. For eg., CallOp is marked with this trait, but if a memref argument for the callee isn't normalizable due to a user op in that callee where it's not normalizable, that call op won't be normalized and transitively other ops won't be as well.

In the future or as a TODO, I think the right way to model this is using SideEffects, in particular, MemoryEffects (I briefly mentioned this on thread). This allows you to specify an effect at operand granularity. The design will require more thought though.

Implemented all of the requested changes. Introduced at TODO for partially normalizable operations.

added proper order for traits

Added revisions suggested for comments.

Added requested doc, edited the summary to be more specific with respect to the contribution of this patch, and removed a prior TODO with respect to recognizing operations that can be normalized by systematically using the new trait.

remove spurious commented code

This functionality makes it very straightforward to develop SIMD code and replicate at a high level the ability to view memory as either scalar or SIMD code with very few restrictions.

Once this patch is in, I have a follow on patch that introduces a new trait, MemRefNormalizable to be used by operations (such as DeallocOp, CallOp, ReturnOp) as well as any ops, for example, such as operations of an dialect that 1) will be expanded to external calls at some later time and 2) are ok with having their memrefs normalized to get rid of their maps.

LGTM

Looking into extending this approach to handle dialect ops that will become calls to external functions. Specifically, I am looking for a way to express that an operation guarantees that (when it will be lowered to an external function call), the external function be aware of the data layout of the maps and thus it is safe to normalize the op memref parameters.

@bondhugula Static is easy. We are working on making it work for the dynamic shapes, currently in our code base but we could migrate it to the MLIR as part of another patch. Basically, we go back to the alloc responsible for the array, and parse the memref to pick either a constant or the right alloc parameter corresponding to the n th '?' in the memref of the alloc.

I tried the patch and this simple code appears to disable memref normalization

The code looks good, and I like the addition of skipping the analysis for "extern" functions. Because dialect ops can in some ways also represent "future" external functions, a natural way of handling "not yet expanded" external functions would to add an interface query that ask if a given op is "external" also. Default would be false, and dialects that needs dome/all of their ops to be external would be able to refine the interface to provide the necessary functionality.

The bug was on my side, I made it more generic. All is good.

In D84490#2179268, @avarmapml wrote:

@AlexEichenberger : Such operation handling can be a part of subsequent revision.
Thank you.

This patch does the advertised functionality well, as shown with the little example below:

sorry, requested a revision by mistake. Repost as a comment below

Generalized the test, can be easily modified to test other dimensions as needed

code is ready to be dropped, since I don't have commit privilege, can you please assist, thanks.

Addressed latest spelling requests. I do need help to commit changes, did a few sometimes ago, not sure how to do it now.

now all comments are done.

latest comments addressed

Added the requested lit tests

I added the { and } around the if statement specifically to avoid name conflicts. I don't like assignment in if-conditional, but since it's both legal and machine generated code (not intended for human consumption), I think it's ok.

What do you suggest I do? Would you like me to introduce the get and set, and leave the older methods for deprecation? Or only add the setter with "set" in the name?

tx for the valuable input

added tests of setters, removed unneeded code, edited & migrated comments as requested

updated style

LGTM

LGTM

LGTM

fantastic

response to Alexey's comment

LGTM, just made one minor suggestion

see note above; apologies if it is already done and hiding somewhere I did not see

See suggested changes, should be pretty straightforward to do, let me know if you need help

see comments

make sense, thanks

Yes, we should have done this right away. I did a rewrite of the functionality that uses header files already included; it was posted in D50522 and the changes were approved and pushed to svn.

• Merge branch 'unpatched-master' into unpatched-master-target-offload-envvar
• while (0) edit

• added do while(1) for macros

• put fatal message for default tgt test

Alexey was not happy about adding an include, and we already used the define approach for the debug messages. So we reuse the same functionality that we already used for debug.
Agreed that Johnas suggested it, and given the problem with the include, Alexey urged me to change the code to what we have here.

In D51226#1212685, @Hahnfeld wrote:

I don't understand this: In the last review you said that we need the complicated FatalMessage and now you are reverting that?!?

• format

I reworked the solution to avoid varargs, and proposed a more robust solution in D51226