[PATCH] D47188: Intel SVML calling conventions

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: include/llvm/IR/SVML.td:53
+// While SVML doesn't provide _ha versions of
+// the following symbols let's disable their vectorization.
+
----------------
dvnagorny wrote:
> hfinkel wrote:
> > dvnagorny wrote:
> > > hfinkel wrote:
> > > > What does this mean? Why would a _ha variant of floor, for example, be needed for vectorization? Or, to put it another way, how would a _ha variant of floor differ from the _ep version?
> > > Really I don't expect any difference in behaviour, However I think that link-time weak aliases for these symbols are more preferable here. It will not require any additional logic in function name mangling code.
> > >  
> > Do these aliases exist currently? I'm concerned about the comment saying that we're disabling vectorization of floor, etc., and I'm prefer that we not disable vectorization unnecessarily.
> Really this code will work when vector-library SVML option will be explicitly passed. So it doesn't disable any default vectorizations. On the other hand in the current LLVM state these function's vectorization isn't enabled yet. So I keep their vectorization the same. From SVML library side aliases aren't available yet however this library not the only potential source of aliases. 
> It's quite simple to provide them from the application code.
Okay, but providing them in application code doesn't help autovectorization, does it? Passing -fveclib=SVML should enable autovectorization of calls to floor, etc. You're saying that it doesn't now, and so I can certainly see that it might be best to change that in a separate patch, but saying that we'll wait for aliases to appear in the library doesn't sound like a good plan (and doesn't help users of existing versions of the library). The vectorizer can call some version which is already there.

In short, would it make sense for the comment to read?

  // TODO: SVML does not currently provide _ha varients of these fucnctions. We should call the _ep variants of these functions in all cases instead.



Repository:
  rL LLVM

https://reviews.llvm.org/D47188