Remove checking for training specific parameters in EmbeddingBag lowering#3782
Conversation
|
Since I don't have permission to add reviewers, I am tagging @vidsinghal here. Vidush, Could you please take a look at this change? |
|
LGTM. |
Thank you for looking into this. Since I don't have permission to merge this change, could you please help merge it? |
I will merge it when all the tests pass. |
I needed to run |
Ah ok, seems like someone already merged it. |
My colleague @rafaelubalmw has merge access and he did the merge. Thanks again, Vidush. |
| Value scaleGradByFreq = op.getScaleGradByFreq(); | ||
| Value mode = op.getMode(); | ||
| Value sparse = op.getSparse(); |
There was a problem hiding this comment.
Hi @Hanumanth04, isn't silently removing those operands from the code wrong? I mean it's fine that we're not making use of those args but let's say for an instance of this op if those args are present then wouldn't the behavior of this IR be wrong or not as expected in that case?
There was a problem hiding this comment.
Hi @vivekkhandelwal1 ,
These are training-only specific arguments, and the existing implementation doesn't have the lowering for training-specific logic. We can introduce these arguments back if we ever decide to add lowering logic for training. However, I don't know if there is a use case for this though. Please note that I have only removed these arguments from the lowering as they were unused after my change. However, the tablegen file corresponding to this operator still has these arguments. I believe removing these unused arguments is not an issue here.
There was a problem hiding this comment.
Hi @Hanumanth04, since they are training-specific args how come they were set for instances where only eval/inference is done? I'm not able to understand if they are only training specific args then any code doing inference should not be setting them and if that's not true then the issue is where that PyTorch code is written at higher level not at the lowering level.
4 participants
Torch-to-linalg pass fails for
EmbeddingBagwhen the training only specific properties of the operator are set totrue.For instance, this operator'ssparseinput/property is training-specific, and if the value of this property istrue,the existing lowering bails out. However, we don't need to check for training-specific parameters and bailout from the legalization since we don't care about these properties during the eval/inference mode.